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

chore: fix deadlock in influx_inspect dumptsi #22661

Merged
merged 9 commits into from
Oct 20, 2021

Conversation

serenibyss
Copy link
Contributor

@serenibyss serenibyss commented Oct 13, 2021

Closes #22658

This PR fixes the deadlock demonstrated in EAR 2636 by swapping the order of fs.Close() and fs.Release(), which due to how Go's defer keyword works, Close() was being called before Release(), and Close() was calling SyncGroup.Wait(), causing the deadlock.

Additionally, I improved some of the error logging for this command (most of which comes from the 2.x version of this command), and improved some of our synchronization object handling in various IndexFile and LogFile functions

@lesam lesam self-requested a review October 13, 2021 18:44
@serenibyss serenibyss force-pushed the ds-fix-dumptsi-deadlock branch from b65d72b to c9093c4 Compare October 13, 2021 18:44
Copy link
Contributor

@lesam lesam left a comment

Choose a reason for hiding this comment

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

Looked at it with @DStrand1 offline - we should make sure the reference counting is correct, and also add a test.

@serenibyss serenibyss requested a review from lesam October 19, 2021 19:39
@serenibyss serenibyss force-pushed the ds-fix-dumptsi-deadlock branch from 464299b to 318d580 Compare October 19, 2021 20:30
lesam
lesam previously approved these changes Oct 19, 2021
pkg/tar/untar.go Outdated Show resolved Hide resolved
@serenibyss serenibyss requested a review from lesam October 19, 2021 21:31
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

We need to catch the unhandled errors. Close() can fail from things like flushing buffers.

@lesam had a nice little idiom for doing this; you can see it here:

https://github.com/influxdata/edge/blob/10a6d3dba9b08c67183519a1023f0bd44989af4c/cmd/oom-decoy/main.go#L36

cmd/influx_inspect/dumptsi/dumptsi.go Outdated Show resolved Hide resolved
cmd/influx_inspect/dumptsi/dumptsi.go Outdated Show resolved Hide resolved
cmd/influx_inspect/dumptsi/dumptsi_test.go Outdated Show resolved Hide resolved
pkg/tar/untar.go Outdated Show resolved Hide resolved
pkg/tar/untar.go Outdated Show resolved Hide resolved
pkg/tar/untar.go Outdated Show resolved Hide resolved
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

LGTM

@serenibyss serenibyss merged commit 06d1df2 into master-1.x Oct 20, 2021
@serenibyss serenibyss deleted the ds-fix-dumptsi-deadlock branch October 20, 2021 17:49
chengshiwen pushed a commit to chengshiwen/influxdb that referenced this pull request Aug 11, 2024
chengshiwen pushed a commit to chengshiwen/influxdb that referenced this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants