nrxus / faux

Struct mocking library for Rust
https://nrxus.github.io/faux/
MIT License
411 stars 14 forks source link

Fix a segfault in release mode #42

Closed coolreader18 closed 3 years ago

coolreader18 commented 3 years ago

Yes, that change fixes a segfault.

Turns out that in release mode some of the _faux_originalname functions were getting deduplicated, since they have the same inputs, same body, and always panic. So when they were cast to usize to use as a key in MockStore, they would point to the same Vec<Mock> despite being from different functions with different return types. Just changing the body to point to a different string literal is enough to prevent the deduplication, though running CI tests in release mode would probably be good to catch this if more optimizations get released. (Or use something like a per-method once_cell::race::OnceNonZeroUsize and a global counter to bypass function ids entirely)

nrxus commented 3 years ago

Oh wow, great find! Thank you!

Indeed one of my current tests were already failing in release mode, and this fixes it! 🎉

I don't quite get your suggestion about using OnceNonZeroUSize. Could you elaborate?

coolreader18 commented 3 years ago

Like, instead of using a Self::_faux_foo as usize, have something like:

fn _faux_id_foo() -> usize {
    static ID: faux::LazyId = faux::LazyId::new();
    ID.get()
}

And then LazyId is just a low-cost OnceCell-like thing, since it doesn't matter if there's a race to fetch the id since a GLOBAL_COUNTER: AtomicUsize would always only go up.

once_cell::race module description:

“First one wins” flavor of OnceCell. If two threads race to initialize a type from the race module, they don’t block, execute initialization function together, but only one of them stores the result.

coolreader18 commented 3 years ago

Also, would it be a good idea to yank 0.1.0 + 0.1.1?

nrxus commented 3 years ago

I don't think it is necessary to yank any versions. Yanking is only ever needed if there are big security concerns/implications of using that older version. A bug in a testing library doesn't qualify as a big security implication in my opinion.