Skip to content

contrib/Shopify/sarama: Fix possible deadlock in WrapAsyncProducer #907

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 2 commits into from
Apr 21, 2021
Merged

contrib/Shopify/sarama: Fix possible deadlock in WrapAsyncProducer #907

merged 2 commits into from
Apr 21, 2021

Conversation

pellared
Copy link
Contributor

The client can still write to the input channel after the goroutine created by WrapAsyncProducer was already shut down. Therefore the channel should be closed. Otherwise, it will deadlock.

For references, it initially fixed here: signalfx/signalfx-go-tracing#128

I had some very dirty test to reproduce it locally but it is not worth putting to the codebase. The test was complex and the bug is pretty obvious.

@gbbr gbbr changed the title Fix possible deadlock in sarama instrumentation WrapAsyncProducer contrib/Shopify/sarama: Fix possible deadlock in sarama instrumentation WrapAsyncProducer Apr 20, 2021
@gbbr
Copy link
Contributor

gbbr commented Apr 20, 2021

Can you please add a regression test for this use case?

@pellared
Copy link
Contributor Author

pellared commented Apr 20, 2021 •
edited
Loading

Can you please add a regression test for this use case?

Not in upcoming days. Feel free to add it on your own.

PS. Would you also like to have tests to ensure that other channels are closed as well. I do not think these kind of tests make much sense 😉

@gbbr
Copy link
Contributor

gbbr commented Apr 20, 2021

PS. Would you also like to have tests to ensure that other channels are closed as well. I do not think these kind of tests make much sense 😉

I was referring to a test reproducing the deadlock.

@pellared
Copy link
Contributor Author

pellared commented Apr 20, 2021 •
edited
Loading

I was referring to a test reproducing the deadlock.

My personal opinion is that having tests for such stuff isn't any good.
This is rather a place for linters to catch such kind of issues. Maybe there is already a linter that would find such an issue? Personally, I have no idea. You can try exploring https://golangci-lint.run/usage/linters/ . Maybe it would detect more issues in this repository.

I have created a little program that can be used to find concurrency issues connected with both sarama and its instrumentation: https://github.com/pellared/dd-sarama-deadlock-example/blob/409022c77d58d2f4cdb0d5547dc9782b70849c2f/main.go

I am able to write a test to detect it however it would assume some internal details and the test may stop working if the implementation changes. I just want to ensure that you REALLY want it. I imagine someone would make some internal changes and then this test would break and a developer would spend an hour (or more) to figure out what this test does (it would have similar complexity to this.

P.S. I really have good intentions. If this would not be true, I would simply add a test or even not create this PR in the first place 😄

@pellared pellared changed the title contrib/Shopify/sarama: Fix possible deadlock in sarama instrumentation WrapAsyncProducer contrib/Shopify/sarama: Fix possible deadlock in WrapAsyncProducer Apr 20, 2021
gbbr
gbbr approved these changes Apr 20, 2021
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I see what you mean. It makes sense. Fine by me. Thanks for pushing this change and for explaining.

@gbbr gbbr added this to the 1.30.0 milestone Apr 20, 2021
@pellared pellared mentioned this pull request Apr 20, 2021
Merged
@gbbr gbbr merged commit 37a411d into DataDog:v1 Apr 21, 2021
@pellared pellared deleted the patch-1 branch April 21, 2021 07:38
stlimtat pushed a commit to stlimtat/dd-trace-go that referenced this pull request May 17, 2021
Merge branch 'v1' of https://github.com/DataDog/dd-trace-go into v1
fabf2b9 
* 'v1' of https://github.com/DataDog/dd-trace-go:
  Implement DD_PROFILING_OUTPUT_DIR for dev/debug (DataDog#924)
  contrib/go-pg/pg.v10: add INTEGRATION flag check for tests (DataDog#921)
  contrib/go-redis/redis.v8: optimize BeforeProcess and BeforeProcessPipeline (DataDog#920)
  CI: check go.sum is up-to-date (DataDog#918)
  README: Fix go get instructions (DataDog#913)
  internal/log: Improve API for testing (DataDog#916)
  ddtrace/tracer: add support for agent discovery endpoint, feature flags, stats & drops (DataDog#859)
  internal/version: bump to v1.31.0 (DataDog#910)
  CONTRIBUTING.md: add link to contrib/README.md (DataDog#802)
  profiler: Disable agentless mode for WithAPIKey (DataDog#906)
  contrib/Shopify/sarama: fix possible deadlock in WrapAsyncProducer (DataDog#907)
  contrib/database/sql.tracedConn implement driver.SessionResetter (DataDog#908)
  ddtrace/opentracer: consider FollowsFrom references as children (DataDog#905)
  ddtrace/opentracer: add support for opentracing.TracerContextWithSpanExtension (DataDog#855)
  ddtrace/tracer: follow noDebugStack setting when using SetTag with an error (DataDog#900)
  contrib/net/http: add a getter to retrieve original round tripper (DataDog#903)
  ddtrace/tracer: ensure Flush call is synchronous (DataDog#901)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbbr gbbr gbbr approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@pellared @gbbr