Skip to content

Prevent async requests from hanging when not instrumented #728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Mar 18, 2025
Merged

Prevent async requests from hanging when not instrumented #728

merged 3 commits into from
Mar 18, 2025

Conversation

batuhansk
Copy link
Contributor

@batuhansk batuhansk commented Mar 13, 2025 •
edited
Loading

This PR fixes an issue where async requests would hang indefinitely when configured to not be instrumented via shouldInstrument returning false. While testing against the current implementation, I discovered that synchronous requests worked correctly, but async requests consistently failed by hanging indefinitely.

Key Changes:

  1. Fix Implementation:

    • Added nil check before modifying task's currentRequest in Task context

  2. Testing:

    • Updated shouldInstrument path/host check logic
    • Fixed HttpTestServer to handle non-instrumented paths properly
    • Added comprehensive test suite after reproducing the issue
    • Verified synchronous requests already worked correctly
    • Added new tests for async/await requests in various Task contexts
    • Confirmed all test cases pass with the fix, including:
      • Regular synchronous requests
      • Async/await requests
      • Detached Task contexts
      • Explicit Task contexts
    • Verified no instrumentation is added when requests are configured to be skipped

Fixes #726

@batuhansk
Enhance URLSessionInstrumentation to handle non-instrumented requests…
5008e8d 
… and improve request logging

- Updated URLSessionInstrumentation to conditionally set the currentRequest only if the instrumentedRequest is valid.
- Modified the shouldInstrument logic to check for specific paths and hosts for non-instrumented requests.
- Added comprehensive tests to verify the behavior of non-instrumented requests, ensuring they complete successfully without instrumentation interference.
- Updated HttpTestServer to respond correctly to non-instrumented request paths.
nachoBonafonte
nachoBonafonte approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@nachoBonafonte nachoBonafonte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good to me, thanks a lot for the tests covering all the scenarios.

@nachoBonafonte nachoBonafonte merged commit 7e7f7f0 into open-telemetry:main Mar 18, 2025
10 checks passed
@bryce-b bryce-b mentioned this pull request Mar 20, 2025
Closed
@batuhansk
Copy link
Contributor Author

Thank you @nachoBonafonte @bryce-b Could you also share a new release? That might be a hotfix and could affect most apps that use the SDK based on their configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nachoBonafonte nachoBonafonte nachoBonafonte approved these changes

@bryce-b bryce-b bryce-b approved these changes

@vvydier vvydier Awaiting requested review from vvydier vvydier is a code owner

@ArielDemarco ArielDemarco Awaiting requested review from ArielDemarco ArielDemarco is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URLSessionInstrumentation causes BrainTree SDK requests to hang indefinitely
3 participants
@batuhansk @nachoBonafonte @bryce-b