GeezerVison

Streaming span leaks if setup fails before guarded try

Move _raw_stream access (and the other pre-try setup) into the existing try/except so finish() still runs if instrumentation setup raises after the child span is created.

_{Identified by Warden · code-review, find-bugs · HDS-MQ3}

I'll check this out in the next PR, cause I don't want to have merge issues

Fix attempt detected (commit ff199b0)

The commit clearly attempts to harden streaming span finalization, but _raw_stream access and other setup still occur before the guarded try, so an exception there can still leak the child span.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Fix attempt detected (commit 11ea5b8)

The change clearly attempts to harden streaming span finalization, but body._raw_stream and related setup still execute before the try/except that calls finish_span, so a setup exception can still leak the child span.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Fix attempt detected (commit 65dfa57)

The change clearly attempts to harden streaming-span finalization, but accesses to body._raw_stream and raw_stream.close still occur before the try/except, so setup failures there can still leak the child span.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Fix attempt detected (commit 4892911)

The change adds guarded cleanup for instrumentation assignments, but _raw_stream access and other setup remain before the try, so an exception there still leaks the child span.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Fix attempt detected (commit d7a3959)

The change clearly attempts to finalize the streaming span on setup failures, but accesses to body.read, body.close, body._raw_stream, and raw_stream.close remain before the guarded try, so an exception there still leaks the span.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Fix attempt detected (commit 09a038e)

The change clearly targets streaming-span finalization and adds cleanup for failures during wrapper assignment, but accessing body.read, body.close, and body._raw_stream.close still occurs before the guarded try and before finish_span is defined, so a setup exception can still leak the child span.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Fix attempt detected (commit 364f547)

The change clearly hardens streaming-span finalization, but _raw_stream access and other setup still occur before the guarded try, so a setup exception can leave the child span unfinished.

The original issue appears unresolved. Please review and try again.

_{Evaluated by Warden}

Why return a boolean here when we're not using the result?

I probably messed up when splitting the large PR in smaller ones; in the next PR of the stack, we'll use this return value to check whether there was 1. a StreamingBody as response and 2. it was instrumented; since we only want to delay closing the boto span if both conditions are met.

I'll document this better in the next PR.

Couple of variable name suggestions to improve readability:

ret => read_return_value
amount => amount_of_bytes_requested

Not specific to your changes and more of a general note on why I'm suggesting this (and other) renames:

We have more than a few vague names within the SDK - both within the "working code" and our tests. This makes it difficult to understand what exactly is being invoked when calling a method, what data is represented by a variable, or what behaviour we're looking to test.

I'd like us to try and move a bit more in the direction of being more specific so that someone reading this in the future doesn't have to jump to function definitions or ask a clanker what pieces of data mean or what's being tested (at least as often as we may have to now).

good to know, I'll try to keep this in mind in the future.

When these changes land, we're going to have to make sure that we remove the branching on span_streaming in the major/3.0 branch 😅

Yupp, there are quite a lot of these cases 😬