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

Thread Sanitizer - Data race reserve_inner #540

Closed
drconopoima opened this issue Mar 26, 2022 · 9 comments · Fixed by #541
Closed

Thread Sanitizer - Data race reserve_inner #540

drconopoima opened this issue Mar 26, 2022 · 9 comments · Fixed by #541

Comments

@drconopoima
Copy link

Hello, I wish you a nice day

I have run my personal project newsletter-rs

https://github.com/drconopoima/newsletter-rs

through thread sanitizer as detailed here https://doc.rust-lang.org/beta/unstable-book/compiler-flags/sanitizer.html

I'm spawning a thread blocking to perform tokio-postgres cached statement queries in background to continuously assert the health of the postgres database.

It detects the data race at this line, on dependency Bytes 1.1.0

https://github.com/sfackler/rust-postgres/blob/38da7fa8fe0067f47b60c147ccdaa214ab5f5211/postgres-protocol/src/message/backend.rs#L115

I'm able to reproduce reliably like:

git clone https://github.com/drconopoima/newsletter-rs --branch bytes_reserve --depth 1
cd newsletter-rs
export RUSTFLAGS=-Zsanitizer=thread RUSTDOCFLAGS=-Zsanitizer=thread
cargo +nightly run -Zbuild-std --target x86_64-unknown-linux-gnu

The complete thread sanitizer output:

threadsanitizer.log

@Darksonn
Copy link
Contributor

This is because thread sanitizer does not support atomic fences. It was earlier discussed in #405, but that PR was closed because detecting use of TSAN was unstable. I'm not sure if it is still unstable. I'll try to open a PR that fixes, and then we will see if our minimum-supported-rust-version CI check complains about it being unstable.

@Darksonn
Copy link
Contributor

@drconopoima Are you able to try out the change in the PR? You can use it as follows:

[patch.crates-io]
bytes = { git = "https://github.com/tokio-rs/bytes", branch = "tsan-support" }

@drconopoima
Copy link
Author

I run it, I no longer see any warnings on bytes_mut.rs:1249.

But I receive a warning when upon exiting the server with Ctrl+C in another line: bytes_mut.rs:1269

#2 bytes::bytes_mut::release_shared::h5b9c07aa69b9828a /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/bytes_mut.rs:1269:8 (newsletter-rs+0x10eb094) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #3 bytes::bytes_mut::shared_v_drop::_$u7b$$u7b$closure$u7d$$u7d$::h67b16f652683dd4a /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/bytes_mut.rs:1579:9 (newsletter-rs+0x10eb66e) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #4 _$LT$core..sync..atomic..AtomicPtr$LT$T$GT$$u20$as$u20$bytes..loom..sync..atomic..AtomicMut$LT$T$GT$$GT$::with_mut::h6c5c3ad7841bba3c /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/loom.rs:17:17 (newsletter-rs+0x10f0e98) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #5 bytes::bytes_mut::shared_v_drop::h9dd284be02738099 /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/bytes_mut.rs:1578:5 (newsletter-rs+0x10eb613) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #6 _$LT$bytes..bytes..Bytes$u20$as$u20$core..ops..drop..Drop$GT$::drop::hf2b210378a0da7f2 /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/bytes.rs:515:18 (newsletter-rs+0x10eca40) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #7 core::ptr::drop_in_place$LT$bytes..bytes..Bytes$GT$::h3ab151872d1f9a34 /home/ljdm/snap/rustup/common/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:188:1 (newsletter-rs+0x10ef721) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #7 bytes::bytes_mut::release_shared::h5b9c07aa69b9828a /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/bytes_mut.rs:1299:23 (newsletter-rs+0x10eb0fc) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #8 _$LT$bytes..bytes_mut..BytesMut$u20$as$u20$core..ops..drop..Drop$GT$::drop::hf47cd8e4621ef839 /home/ljdm/.cargo/git/checkouts/bytes-dba3028afccc9a98/db46e3b/src/bytes_mut.rs:970:22 (newsletter-rs+0x10ead07) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)
    #9 core::ptr::drop_in_place$LT$bytes..bytes_mut..BytesMut$GT$::h9d70b23a637d3133 /home/ljdm/snap/rustup/common/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:188:1 (newsletter-rs+0x10ef815) (BuildId: 76981792c1d5bda900edca585e71ff75d1cd52a3)

threadsanitizer20220329.txt

@Darksonn
Copy link
Contributor

I'm unable to reproduce that. When I run the commands you posted earlier, it just prints a bunch of json and then exits normally.

@drconopoima
Copy link
Author

My excuses @Darksonn, I forgot to mention the postgres container dependency.

I prepared a new branch, patch tsan-support applied and the database command

git clone https://github.com/drconopoima/newsletter-rs --branch bytes --depth 1
cd newsletter-rs
docker run -d --name newsletter-rs-db --rm -e POSTGRES_USER=postgres -e POSTGRES_PASSWORD=password -p 5432:5432 --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 docker.io/library/postgres:14 postgres -N 1000
export RUSTFLAGS=-Zsanitizer=thread RUSTDOCFLAGS=-Zsanitizer=thread
cargo +nightly run -Zbuild-std --target x86_64-unknown-linux-gnu

As soon as logs stop coming, a Ctrl +C and the warning is emitted by the Thread Sanitizer

Thank you for your help.

@Darksonn
Copy link
Contributor

I just pushed a change to the tsan-support branch which appears to resolve the issue when I test locally. Does it also resolve the issue for you?

@drconopoima
Copy link
Author

Thank you. I have tested as well from the latest commit tsan-support#017b8a7e089 and all warnings have dissapeared

@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2022

@drconopoima Are you able to test again with the latest version?

@drconopoima
Copy link
Author

Not giving me any problems the last patch

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 a pull request may close this issue.

2 participants