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

fix: prevent exceptions in callbacks from causing deadlocks #559

Merged
merged 5 commits into from
Feb 13, 2023

Conversation

btasker
Copy link
Contributor

@btasker btasker commented Feb 9, 2023

Closes #558

Proposed Changes

Introduces two levels of deadlock protection

  • Traps exceptions thrown by callbacks and logs a warning
  • Implements a maximum wait time during close()

The second change involves the introduction of a new WriteOption: max_close_wait. The default is set to 5 minutes, but can be overriden in the same manner as other write options.

Note: in order to allow the failure to be tested safely, pytest-timeout has been added as a test dependency.

Checklist

  • [x ] CHANGELOG.md updated
  • [x ] Rebased/mergeable
  • [x ] A test has been added if appropriate
  • [x ] pytest tests completes successfully
  • [x ] Commit messages are conventional
  • [x ] Sign CLA (if not already signed)

This is one part of a fix for influxdata#558

It:

* Adds a WriteOption `max_close_wait` (default 500,000ms)
* Adjust `__del__` so that we'll only wait `max_close_wait`ms for queued writes to complete
* Adds a warning if the threshold is hit
The other part of the fix for influxdata#558

If a configured callback results in an exception we'll trap it and log
the details rather than allowing it to interrupt the parent thread
This introduces a new testing dependency: `pytest-timeout`. As the issue
being tested for can cause deadlocks, it seemed prudent to avoid tests
locking up
@powersj powersj requested a review from srebhan February 9, 2023 20:30
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the fix @btasker!

@srebhan srebhan merged commit 497738c into influxdata:master Feb 13, 2023
@bednar bednar added this to the 1.35.0 milestone Jul 28, 2023
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.

Exceptions in Callback Handlers can lead to deadlock
4 participants