sync: ensure Mutex, RwLock, and Semaphore futures are Send + Sync #2375
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Previously, the `Mutex::lock`, `RwLock::{read, write}`, and
`Semaphore::acquire` futures in `tokio::sync` implemented `Send + Sync`
automatically. This was by virtue of being implemented using a `poll_fn`
that only closed over `Send + Sync` types. However, this broke in
PR #2325, which rewrote those types using the new `batch_semaphore`.
Now, they await an `Acquire` future, which contains a `Waiter`, which
internally contains an `UnsafeCell`, and thus does not implement `Sync`.
Since removing previously implemented traits breaks existing code, this
inadvertantly caused a breaking change. There were tests ensuring that
the `Mutex`, `RwLock`, and `Semaphore` types themselves were `Send +
Sync`, but no tests that the _futures they return_ implemented those
traits.
I've fixed this by adding an explicit impl of `Sync` for the
`batch_semaphore::Acquire` future. Since the `Waiter` type held by this
struct is only accessed when borrowed mutably, it is safe for it to
implement `Sync`.
Additionally, I've added to the bounds checks for the effected
`tokio::sync` types to ensure that returned futures continue to
implement `Send + Sync` in the future.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
LucioFranco
approved these changes
Apr 3, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
I think we probably should do a version bump — @carllerche, what do you think? I think that should be staged in a separate PR though. |
carllerche
approved these changes
Apr 3, 2020
|
@hawkw cool, if you can open a bump Pr I can do the rest. |
|
We should push a patch release. You can include the version bump / changelog here too. |
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
|
@carllerche okay, added the version bump in 89fcffb |
carllerche
requested changes
Apr 3, 2020
whoops, thanks carl! Co-Authored-By: Carl Lerche <me@carllerche.com>
|
Whoops, thanks for catching my typo @carllerche! +1 for obeying time's arrow :) |
carllerche
approved these changes
Apr 3, 2020
|
It would be nice if azure windows CI stopped randomly not reporting back.... |
|
@carllerche looks like it's good now? |
hawkw
added a commit
that referenced
this pull request
Apr 4, 2020
# 0.2.16 (April 3, 2020) ### Fixes - sync: fix a regression where `Mutex`, `Semaphore`, and `RwLock` futures no longer implement `Sync` (#2375) - fs: fix `fs::copy` not copying file permissions (#2354) ### Added - time: added `deadline` method to `delay_queue::Expired` (#2300) - io: added `StreamReader` (#2052) Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Previously, the
Mutex::lock,RwLock::{read, write}, andSemaphore::acquirefutures intokio::syncimplementedSend + Syncautomatically. This was by virtue of being implemented using a
poll_fnthat only closed over
Send + Synctypes. However, this broke inPR #2325, which rewrote those types using the new
batch_semaphore.Now, they await an
Acquirefuture, which contains aWaiter, whichinternally contains an
UnsafeCell, and thus does not implementSync.Since removing previously implemented traits breaks existing code, this
inadvertantly caused a breaking change. There were tests ensuring that
the
Mutex,RwLock, andSemaphoretypes themselves wereSend + Sync, but no tests that the futures they return implemented thosetraits.
Solution
I've fixed this by adding an explicit impl of
Syncfor thebatch_semaphore::Acquirefuture. Since theWaitertype held by thisstruct is only accessed when borrowed mutably, it is safe for it to
implement
Sync.Additionally, I've added to the bounds checks for the effected
tokio::synctypes to ensure that returned futures continue toimplement
Send + Syncin the future.Fixes: #2373
Signed-off-by: Eliza Weisman eliza@buoyant.io