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

Add is_locked_serially() to check if we are in a #[serial] context #113

Merged
merged 2 commits into from
Sep 15, 2024

Conversation

pgerber
Copy link
Contributor

@pgerber pgerber commented Jul 2, 2024

I've recently had this code added to rustls-tls-native:

/// Cargo, at least sometimes, sets SSL_CERT_FILE and SSL_CERT_DIR internally,
/// it uses OpenSSL. So, always unset both at the beginning of a test even if
/// the test doesn't use either.
///
/// # Safety
///
/// This is only safe if used together with `#[serial]` because calling
/// `[env::remove_var()]` is unsafe if another thread is running.
///
/// Note that `env::remove_var()` is scheduled to become unsafe in Rust
/// Edition 2024.
pub(crate) unsafe fn clear_env() {
    env::remove_var("SSL_CERT_FILE");
    env::remove_var("SSL_CERT_DIR");
}

clear_env() is called from various tests and it's really easy to forget #[serial]. This change will make it possible
to add an assert!() to ensure we didn't forget:

pub(crate) unsafe fn clear_env() {
    assert!(serial_test::is_locked_serially(None));

    env::remove_var("SSL_CERT_FILE");
    env::remove_var("SSL_CERT_DIR");
}

@palfrey
Copy link
Owner

palfrey commented Jul 3, 2024

Can you add some tests for this please? Particular cases of interest: behaviour inside a serial lock, behaviour outside of a serial lock, behaviour on a lock where's there's more than one name.

You can run the lock functions directly, like in unlock_on_assert (although minus the std::panic bits obviously).

@pgerber
Copy link
Contributor Author

pgerber commented Jul 8, 2024

Looking at the test results, I guess the problem is that I check if there if the lock with given name is locked (or unlocked) but it could be locked by another test/thread.

@palfrey
Copy link
Owner

palfrey commented Jul 9, 2024

Looking at the test results, I guess the problem is that I check if there if the lock with given name is locked (or unlocked) but it could be locked by another test/thread.

Agreed. OTOH, I don't think you need to have the same names in the different tests to be able to do the checks properly which would solve this. Also, in order to do it with the same names would effectively need another serial_test instance to do this one at a time, without using the existing locks!

@pgerber
Copy link
Contributor Author

pgerber commented Jul 12, 2024

Using the same name was quite intentional. The reason for the pull request, as described above, is to be able to use assert!() to ensure that we are holding a serial lock. If I interpret the test result correctly, it's, however, possible that the assert!() passes because another test/thread is holding the lock. I think my implementation is wrong and the tests are are complaining rightfully.

@pgerber
Copy link
Contributor Author

pgerber commented Jul 12, 2024

This implementation should be better, I think. It's hard to tell as I'm having trouble reproducing the issue reported by CI locally.

One thing I've not looked into yet is how this interacts with async.

@palfrey palfrey merged commit 7919b91 into palfrey:main Sep 15, 2024
7 checks passed
@palfrey
Copy link
Owner

palfrey commented Nov 8, 2024

Finally got around to releasing this as part of 3.2.0

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.

2 participants