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 drop order of tls and statics and avoid double panics on deadlock #297

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Pointerbender
Copy link
Contributor

Hello! I ran into some of the same issues as mentioned in #152, #179 and #291 and developed a fix for all of those issues. In a nutshell, the changes are:

  • clarify the documentation of the mock lazy_static implementation regarding its Drop implementation
  • ensure that global statics are dropped after thread locals
  • ensure that loom does not panic inside a panic when dropping thread locals on the main thread
  • add two regression tests for Panic occurs when access lazy_static! at the end of thread #152
  • ensure that thread locals are destructed when a spawned thread panics
  • ensure that loom doesn't double panic on deadlock
  • add a test to verify that thread locals are properly destructed on spawned threads
  • sprinkle comments generously everywhere to explain the code changes

This is my first time contributing to loom, please let me know if I can somehow improve this PR or if additional information is needed. Thank you!

- ensure that global statics are dropped after thread locals
- ensure that `loom` does not panic inside a panic
- add two regression tests for tokio-rs#152
- ensure that loom doesn't double panic on deadlock
- add a test to verify that thread locals are properly destructed on spawned threads
@Pointerbender
Copy link
Contributor Author

Hello! Just checking in to see if there is anything I can do to help shepherd this PR along. :) I'd be happy to provide extra details or hop on chat/zoom to review together if that saves some time. No rush, though!

@hawkw
Copy link
Member

hawkw commented Feb 1, 2023

Thank you for working on this, this is great! I'll try to give it a review at some point soon, although I imagine @carllerche will want to as well.

@Pointerbender
Copy link
Contributor Author

I've addressed the PR comments left by @adrianheine (thanks for the review!), are there any next steps left before this could potentially be merged? :) Please let me know if I can be of any further assistance. Thank you!

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.

3 participants