-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
b65d72b
to
c9093c4
Compare
There was a problem hiding this 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.
464299b
to
318d580
Compare
There was a problem hiding this 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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #22658
This PR fixes the deadlock demonstrated in EAR 2636 by swapping the order of
fs.Close()
andfs.Release()
, which due to how Go'sdefer
keyword works,Close()
was being called beforeRelease()
, andClose()
was callingSyncGroup.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