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

race condition in thread local storage example #58893

Merged
merged 1 commit into from Mar 9, 2019

Conversation

Projects
None yet
4 participants
@benaryorg
Copy link
Contributor

benaryorg commented Mar 3, 2019

The example had a potential race condition that would still pass the test.
If the thread which was supposed to modify it's own thread local was slower than the instruction to
modify in the main thread, then the test would pass even in case of a failure.
This is would be minor if the child thread was waited for since it check using an assert_eq for the
same thing, but vice versa.
However, if the assert_eq failed this would trigger a panic, which is not at all caught by the
example since the thread is not waited on.

Signed-off-by: benaryorg binary@benary.org

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 3, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 3, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:14626d1c:start=1551623687870688286,finish=1551623688862577225,duration=991888939
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:20:17] 
[01:20:17] running 119 tests
[01:20:46] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:20:51] i......iii.i.....ii
[01:20:51] 
[01:20:51]  finished in 34.471
[01:20:51] travis_fold:end:test_debuginfo

---
[01:40:26] .................................................................................................... 600/994
[01:40:34] .................................................................................................... 700/994
[01:40:43] ..............iiii.................................................................................. 800/994
[01:40:56] .................................................................................................... 900/994
[01:41:04] ........................................iiii.F................................................
[01:41:04] 
[01:41:04] ---- thread/local.rs - thread::local::LocalKey (line 31) stdout ----
[01:41:04] ---- thread/local.rs - thread::local::LocalKey (line 31) stdout ----
[01:41:04] error[E0615]: attempted to take value of method `join` on type `std::thread::JoinHandle<()>`
[01:41:04]   --> thread/local.rs:51:3
[01:41:04]    |
[01:41:04] 23 | t.join.unwrap();
[01:41:04]    |   ^^^^ help: use parentheses to call the method: `join()`
[01:41:04] thread 'thread/local.rs - thread::local::LocalKey (line 31)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:352:13
[01:41:04] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:41:04] 
[01:41:04] 
---
[01:41:04] 
[01:41:04] error: test failed, to rerun pass '--doc'
[01:41:04] 
[01:41:04] 
[01:41:04] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:41:04] 
[01:41:04] 
[01:41:04] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:41:04] Build completed unsuccessfully in 0:33:20
[01:41:04] Build completed unsuccessfully in 0:33:20
[01:41:04] Makefile:48: recipe for target 'check' failed
[01:41:04] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:001009aa
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Mar  3 16:16:05 UTC 2019

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

race condition in thread local storage example
The example had a potential race condition that would still pass the test.
If the thread which was supposed to modify it's own thread local was slower than the instruction to
modify in the main thread, then the test would pass even in case of a failure.
This is would be minor if the child thread was waited for since it check using an `assert_eq` for
the same thing, but vice versa.
However, if the `assert_eq` failed this would trigger a panic, which is not at all caught by the
example since the thread is not waited on.

Signed-off-by: benaryorg <binary@benary.org>

@benaryorg benaryorg force-pushed the benaryorg:thread_local_example_join branch from 25e45a4 to 2293d22 Mar 3, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 4, 2019

Thanks for the PR! I think though due to this being thread local storage there's no race condition? Could you elaborate a bit more on the interleaving you're thinking which causes a bug?

@benaryorg

This comment has been minimized.

Copy link
Contributor Author

benaryorg commented Mar 4, 2019

I think though due to this being thread local storage there's no race condition?

Of course there is no race condition in TLS, the problem here is that the test wouldn't catch it if there was one.
Since asserts trigger a panic and the one panic is happening in a thread of which we do not unwrap() the join()s Result, this one goes unnoticed if for some reason the implementation happened to fail (to fail be either "local" or to retaining the default value in the thread to be specific).
Then again there's the race condition which could prevent even the main thread from panicking from its own assert if the thread wasn't running before the main was, since the thread is not being waited on.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 5, 2019

@bors: r+ rollup

Ok, makes sense to me!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 5, 2019

📌 Commit 2293d22 has been approved by alexcrichton

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Mar 8, 2019

Rollup merge of rust-lang#58893 - benaryorg:thread_local_example_join…
…, r=alexcrichton

race condition in thread local storage example

The example had a potential race condition that would still pass the test.
If the thread which was supposed to modify it's own thread local was slower than the instruction to
modify in the main thread, then the test would pass even in case of a failure.
This is would be minor if the child thread was waited for since it check using an `assert_eq` for the
same thing, but vice versa.
However, if the `assert_eq` failed this would trigger a panic, which is not at all caught by the
example since the thread is not waited on.

Signed-off-by: benaryorg <binary@benary.org>

bors added a commit that referenced this pull request Mar 8, 2019

Auto merge of #59012 - pietroalbini:rollup, r=pietroalbini
Rollup of 24 pull requests

Successful merges:

 - #58080 (Add FreeBSD armv6 and armv7 targets)
 - #58204 (On return type `impl Trait` for block with no expr point at last semi)
 - #58269 (Add librustc and libsyntax to rust-src distribution.)
 - #58369 (Make the Entry API of HashMap<K, V> Sync and Send)
 - #58861 (Expand where negative supertrait specific error is shown)
 - #58877 (Suggest removal of `&` when borrowing macro and appropriate)
 - #58883 (Suggest appropriate code for unused field when destructuring pattern)
 - #58891 (Remove stray ` in the docs for the FromIterator implementation for Option)
 - #58893 (race condition in thread local storage example)
 - #58906 (Monomorphize generator field types for debuginfo)
 - #58911 (Regression test for #58435.)
 - #58912 (Regression test for #58813)
 - #58916 (Fix release note problems noticed after merging.)
 - #58918 (Regression test added for an async ICE.)
 - #58921 (Add an explicit test for issue #50582)
 - #58926 (Make the lifetime parameters of tcx consistent.)
 - #58931 (Elide invalid method receiver error when it contains TyErr)
 - #58940 (Remove JSBackend from config.toml)
 - #58950 (Add self to mailmap)
 - #58961 (On incorrect cfg literal/identifier, point at the right span)
 - #58963 (libstd: implement Error::source for io::Error)
 - #58970 (delay_span_bug in wfcheck's ty.lift_to_tcx unwrap)
 - #58984 (Teach `-Z treat-err-as-bug` to take a number of errors to emit)
 - #59007 (Add a test for invalid const arguments)

Failed merges:

 - #58959 (Add release notes for PR #56243)

r? @ghost

bors added a commit that referenced this pull request Mar 8, 2019

Auto merge of #59012 - pietroalbini:rollup, r=pietroalbini
Rollup of 24 pull requests

Successful merges:

 - #58080 (Add FreeBSD armv6 and armv7 targets)
 - #58204 (On return type `impl Trait` for block with no expr point at last semi)
 - #58269 (Add librustc and libsyntax to rust-src distribution.)
 - #58369 (Make the Entry API of HashMap<K, V> Sync and Send)
 - #58861 (Expand where negative supertrait specific error is shown)
 - #58877 (Suggest removal of `&` when borrowing macro and appropriate)
 - #58883 (Suggest appropriate code for unused field when destructuring pattern)
 - #58891 (Remove stray ` in the docs for the FromIterator implementation for Option)
 - #58893 (race condition in thread local storage example)
 - #58906 (Monomorphize generator field types for debuginfo)
 - #58911 (Regression test for #58435.)
 - #58912 (Regression test for #58813)
 - #58916 (Fix release note problems noticed after merging.)
 - #58918 (Regression test added for an async ICE.)
 - #58921 (Add an explicit test for issue #50582)
 - #58926 (Make the lifetime parameters of tcx consistent.)
 - #58931 (Elide invalid method receiver error when it contains TyErr)
 - #58940 (Remove JSBackend from config.toml)
 - #58950 (Add self to mailmap)
 - #58961 (On incorrect cfg literal/identifier, point at the right span)
 - #58963 (libstd: implement Error::source for io::Error)
 - #58970 (delay_span_bug in wfcheck's ty.lift_to_tcx unwrap)
 - #58984 (Teach `-Z treat-err-as-bug` to take a number of errors to emit)
 - #59007 (Add a test for invalid const arguments)

Failed merges:

 - #58959 (Add release notes for PR #56243)

r? @ghost

bors added a commit that referenced this pull request Mar 8, 2019

Auto merge of #59012 - pietroalbini:rollup, r=pietroalbini
Rollup of 24 pull requests

Successful merges:

 - #58080 (Add FreeBSD armv6 and armv7 targets)
 - #58204 (On return type `impl Trait` for block with no expr point at last semi)
 - #58269 (Add librustc and libsyntax to rust-src distribution.)
 - #58369 (Make the Entry API of HashMap<K, V> Sync and Send)
 - #58861 (Expand where negative supertrait specific error is shown)
 - #58877 (Suggest removal of `&` when borrowing macro and appropriate)
 - #58883 (Suggest appropriate code for unused field when destructuring pattern)
 - #58891 (Remove stray ` in the docs for the FromIterator implementation for Option)
 - #58893 (race condition in thread local storage example)
 - #58906 (Monomorphize generator field types for debuginfo)
 - #58911 (Regression test for #58435.)
 - #58912 (Regression test for #58813)
 - #58916 (Fix release note problems noticed after merging.)
 - #58918 (Regression test added for an async ICE.)
 - #58921 (Add an explicit test for issue #50582)
 - #58926 (Make the lifetime parameters of tcx consistent.)
 - #58931 (Elide invalid method receiver error when it contains TyErr)
 - #58940 (Remove JSBackend from config.toml)
 - #58950 (Add self to mailmap)
 - #58961 (On incorrect cfg literal/identifier, point at the right span)
 - #58963 (libstd: implement Error::source for io::Error)
 - #58970 (delay_span_bug in wfcheck's ty.lift_to_tcx unwrap)
 - #58984 (Teach `-Z treat-err-as-bug` to take a number of errors to emit)
 - #59007 (Add a test for invalid const arguments)

Failed merges:

 - #58959 (Add release notes for PR #56243)

r? @ghost

bors added a commit that referenced this pull request Mar 9, 2019

Auto merge of #59012 - pietroalbini:rollup, r=pietroalbini
Rollup of 24 pull requests

Successful merges:

 - #58080 (Add FreeBSD armv6 and armv7 targets)
 - #58204 (On return type `impl Trait` for block with no expr point at last semi)
 - #58269 (Add librustc and libsyntax to rust-src distribution.)
 - #58369 (Make the Entry API of HashMap<K, V> Sync and Send)
 - #58861 (Expand where negative supertrait specific error is shown)
 - #58877 (Suggest removal of `&` when borrowing macro and appropriate)
 - #58883 (Suggest appropriate code for unused field when destructuring pattern)
 - #58891 (Remove stray ` in the docs for the FromIterator implementation for Option)
 - #58893 (race condition in thread local storage example)
 - #58906 (Monomorphize generator field types for debuginfo)
 - #58911 (Regression test for #58435.)
 - #58912 (Regression test for #58813)
 - #58916 (Fix release note problems noticed after merging.)
 - #58918 (Regression test added for an async ICE.)
 - #58921 (Add an explicit test for issue #50582)
 - #58926 (Make the lifetime parameters of tcx consistent.)
 - #58931 (Elide invalid method receiver error when it contains TyErr)
 - #58940 (Remove JSBackend from config.toml)
 - #58950 (Add self to mailmap)
 - #58961 (On incorrect cfg literal/identifier, point at the right span)
 - #58963 (libstd: implement Error::source for io::Error)
 - #58970 (delay_span_bug in wfcheck's ty.lift_to_tcx unwrap)
 - #58984 (Teach `-Z treat-err-as-bug` to take a number of errors to emit)
 - #59007 (Add a test for invalid const arguments)

Failed merges:

 - #58959 (Add release notes for PR #56243)

r? @ghost

@bors bors merged commit 2293d22 into rust-lang:master Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.