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: make sure to join all arbiters #3932

Closed

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Feb 9, 2021

fixes #3925

It is a combination of two effects.

First, we are using rocks db. It is implemented in C++ and makes use of global
data with desctuctors. Those dtors are run "after main". In practice, "after
main" means "after the main thread is exited". It is thus an error to use
rocksdb instance after main, even if just to destroy it.

Second, we use actix, and it doesn't obey the structured concurrency
discipline ouf of the box. Specifically, if you have a test like this:

#[test]
fn some_test() {
    // do some actix shenanigans
}

it very well might be the case that, after the some_text function exits, there
are some leftover actix therads running!

In combination, this means that the main testing thread might exit first, while
some actix thread is also exiting and droping rocksdb, which will then observe
destroyed global state.

This PR tries to manually wait for actix threads. There's an API to join
Arbiter, which we use. For SyncArbiter, the equivalent API seems to be
missing, so we just busy wait.

TODO:

  • figure out, why did this surface only actix upgrade? The problem seems
    pre-existing.

  • figure out a proper way to join the SyncActor.

  • figure out patterns that make sure that no therads are leaked by construction
    (aka structured concurrency).

@matklad
Copy link
Contributor Author

matklad commented Feb 11, 2021

Ok, this turns out to be significantly more involved, lets continue discussion in #3925

@matklad matklad closed this Feb 11, 2021
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.

CI failure after upgrading actix
1 participant