Skip to content
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.

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

Concurrency fixes #997

Merged
merged 2 commits into from
Aug 5, 2021
Merged

Concurrency fixes #997

merged 2 commits into from
Aug 5, 2021

Conversation

axw
Copy link
Member

@axw axw commented Aug 5, 2021

Fix a deadlock that can occur when concurrently ending a parent and child span, due to the parent waiting for the child to release the transaction lock, and the child waiting to lock the parent. Locks are now taken in a consistent order.

Add synchronisation to the httptrace handlers implemented by the module/apmhttp.WithClientTrace option. Sayeth the httptrace docs:

Functions may be called concurrently from different goroutines and
some may be called after the request has completed or failed.

There's a minor enhancement to the handlers to set the Connect span outcome to "failure" if an error occurred.

Fixes #994

@axw axw force-pushed the concurrency-fixes branch from 8bdb1d8 to f899b95 Compare August 5, 2021 03:43
@axw axw requested a review from a team August 5, 2021 03:44
axw added 2 commits August 5, 2021 11:45
Add synchronisation to requestTracer. Sayeth the httptrace docs:

> Functions may be called concurrently from different goroutines and
some may be called after the request has completed or failed.

There's also a minor enhancement here to set the Connect span outcome
to "failure" if an error occurred.
Fix a deadlock that can occur when concurrently ending
a parent and child span, due to the parent waiting for
the child to release the transaction lock, and the child
waiting to lock the parent. Locks are now taken in a
consistent order.
@axw axw force-pushed the concurrency-fixes branch from f899b95 to c0a1786 Compare August 5, 2021 03:45
@axw axw marked this pull request as ready for review August 5, 2021 03:45
@axw axw mentioned this pull request Aug 5, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Aug 5, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-05T03:45:43.699+0000

  • Duration: 30 min 37 sec

  • Commit: c0a1786

Test stats 🧪

Test Results
Failed 0
Passed 10803
Skipped 268
Total 11071

Trends 🧪

Image of Build Times

Image of Tests

Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

httptrace docs be praised.

@axw axw merged commit cb09aa0 into elastic:master Aug 5, 2021
@axw axw deleted the concurrency-fixes branch August 5, 2021 08:36
blessedvictim pushed a commit to blessedvictim/apm-agent-go that referenced this pull request Aug 10, 2021
* module/apmhttp: add locking to httptrace handlers

Add synchronisation to requestTracer. Sayeth the httptrace docs:

> Functions may be called concurrently from different goroutines and
some may be called after the request has completed or failed.

There's also a minor enhancement here to set the Connect span outcome
to "failure" if an error occurred.

* apm: fix deadlock in breakdown metrics calculation

Fix a deadlock that can occur when concurrently ending
a parent and child span, due to the parent waiting for
the child to release the transaction lock, and the child
waiting to lock the parent. Locks are now taken in a
consistent order.
jsoriano added a commit to elastic/package-registry that referenced this pull request Oct 19, 2021
It includes a fix elastic/apm-agent-go#997 for some concurrency bugs.
Gorilla is indirectly also updated to 1.8.0, but this only includes a fix in a
feature we don't use.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadlock in http.Client
3 participants