-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
v0.1.x: rand, parking_lot and related updates #1358
Conversation
@dekellum I did some playing around with this. Looks like adding |
@LucioFranco This is just a staging draft for proposal #1348. Since the core of that proposal involves cleanup (e.g. avoiding 3 versions of rand) and upgrades to dependencies which increase MSRV to 1.31.0 in any case, it makes sense IMO to actually fix (#1351) rather than just suppress these warnings. That said, if such warnings on the older stable branch v0.1.x of tokio are going to be increasingly common and hard to justify fixing, then perhaps it also makes sense to independently remove |
@dekellum I actually think the deprecated warnings are a bug. I've set it allow which means that it shouldn't even warn but it still warning on nightly. Not only that if I keep deny warnings it still doesn't turn them into errors. so I think its a bug. I'm gonna spend some time this weekend/next week to look over these changes you are proposing. I've started to learn more open to updating the MSRV. But I want to review all the proposed changes and their impacts. Thanks for taking charge on this! |
The CI TSAN errors
...could be related to the rand 0.7 update here and might be a regression in rand_chacha 0.2.1 which was recently released, whith a change to enable simd: https://github.com/rust-random/rand/blob/master/rand_chacha/CHANGELOG.md#021---2019-07-22 |
I can workaround the TSAN failure locally via: |
tsan is pretty sensitive to false positives. In this case, it looks like the issue is in |
For example, tsan doesn't support fences at all, so if lazy_static goes through a fence somehow, it would generate a false positive. |
I went ahead and created a minimized, out of tokio tree, repro for this here: https://github.com/dekellum/rng-tsan-min-repro/blob/master/src/lib.rs In rust-random/getrandom#52 (included in 0.1.7) it looks like some internal state access is relaxed by replacing lazy_static with custom code. I can only guess, that if there is any new/real issue here, that increased concurrency in getrandom has some how exposed a pre-existing issue in rand_chacha? @carllerche thanks for looking and feedback on this. I'd appreciate your opinion on if I should report this on getrandom or if you think I should just suppress it here. Suppressing |
It is really hard for me to tell if the TSAN error is real or a false positive without digging into all the dependencies. You could open an issue to point it out at least to see if they know. |
I had initially figured lazy_static was too commonly used to be the source of TSAN false positives, but it wasn't in the v0.1.x TSAN CI tests before this PR introduced a few. I even isolated a minimal repro for only lazy static, here: https://github.com/dekellum/rng-tsan-min-repro/blob/lazy-static/src/lib.rs The updated c2-chacha (via its ppv-lite86 dep) introduces an additional |
With CI/TSAN passing again, I request this be reviewed more closely as a proof and best considered implementation of the #1348 proposal. |
Use std RandomState for XorShift seeding. This allows dropping _rand_ crate dep here, accept as a dev dependency for tests or benchmarks.
ee8e9fc
to
9073942
Compare
I've rebased and renamed this to reflect its now more limited scope as a set of related dependency updates, MSRV bump to 1.31.0 and fixes to keep TSAN tests working. Its not a "staging" PR anymore, though if merged, release of tokio-threadpool and tokio-reactor are warranted. From the original proposal #1348, this PR retains the items listed below. Again, I would suggest at least keeping this open for consideration if there are future maintenance items requiring releases to these same crates, or if future developments cause additional problems with the old dependencies this removes. (3) Backport #1324: remove last non-dev dependency on rand crateEffects (-threadpool) (4) Upgrade to rand 0.7.0 (dev-only dependency) (testing MSRV 1.32.0)Effects (-fs -threadpool -timer) (5) Backport #1298: Upgrade to parking_lot 0.9.0 (MSRV 1.31.0)Effects (-reactor). Drops rand 0.6 deps. (7) CI change for testing and doc of updated MSRV 1.31.0With rust 1.36.0 stable release, this is a legal MSRV update per README stated policy Net local dev tree dependency changes, including transitive dependencies: Adding c2-chacha v0.2.2
Removing fuchsia-cprng v0.1.1
Adding getrandom v0.1.10
Updating lock_api v0.1.5 -> v0.3.1
Removing owning_ref v0.4.0
Updating parking_lot v0.7.1 -> v0.9.0
Updating parking_lot_core v0.4.0 -> v0.6.2
Adding ppv-lite86 v0.2.5
Updating rand v0.6.5 -> v0.7.0
Updating rand_chacha v0.1.1 -> v0.2.1
Removing rand_core v0.3.1
Removing rand_core v0.4.2
Adding rand_core v0.5.0
Updating rand_hc v0.1.0 -> v0.2.0
Removing rand_isaac v0.1.1
Removing rand_jitter v0.1.4
Removing rand_os v0.1.3
Removing rand_pcg v0.1.2
Removing rand_xorshift v0.1.1
Removing rdrand v0.4.0
Removing scopeguard v0.3.3
Removing stable_deref_trait v1.1.1
Updating tempfile v3.0.8 -> v3.1.0
Adding wasi v0.5.0 |
But avoid tempfile 3.2 for now, since history demonstrates it bumps rand versions and MSRV in MINOR updates.
As shown in #1535, the effective MSRV via build dependencies, at least for use of tokio-tls, is already 1.31.0. |
I'd like to have this, since this is causing a bunch of duplicate dependencies to me. What's needed to move this forward? |
@djc I think we really just needed to see someone wanted this :) I think I am happy to merge this and bump MSRV as users can pin to Thoughts @carllerche I'm 👍 on this? |
Go for it. |
I first the motion. Good timing @djc, as yesterday's cfg-if release has forced the existing 0.1.x (e.g. tokio 0.1.22) crates to MSRV 1.31.0. See last failed CI run in #1535. To pin to some set of crates that will work with rust 1.28, at least without an existing lock file, will now be a difficult task. @LucioFranco: Can I provide you a new staging branch PR that merges this PR with version bumps and CHANGELOG for the two releases: tokio-threadpool and tokio-reactor? CI last passed here over a month ago, so there might be additional breakage to deal with. Also, worth considering: Is there anything else that should go in these releases? |
@dekellum yes please do! I am happy to bump MSRV and release this today/tomorrow. I can't think of anything else as of now. Let me know what I can help with. Thanks <3 |
This was effectively merged as part of #1604. Closing. |
Draft (WIP) for #1348. Initially just for testing and to establish MSRV with passing CI.