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

sync: add split method to the permit types #6472

Merged
merged 4 commits into from
Apr 10, 2024
Merged

Conversation

vvvviiv
Copy link
Contributor

@vvvviiv vvvviiv commented Apr 7, 2024

Add detach method to SemaphorePermit and OwnedSemaphorePermit.

Motivation

Assume there are two tasks named A and B, and we have a Semaphore with two permits.

A needs one permit, B needs two permits at first and one permit later.

If B executes first, it calls acquire_many to get a SemaphorePermit with two permits. After doing some work, one of the permits is unnecessary for B to continue the work, but we can only release all permits at once.

In this case, A has to wait until B finishes all works or B releases its two permits and acquires one permit again to continue the remaining work without blocking A.

Since we have the merge method, I believe that the corresponding inverse operation is reasonable.

Solution

Adds the detach method to SemaphorePermit and OwnedSemaphorePermit to allow us to detach some permits from the source permits.

Add `detach` method to `SemaphorePermit` and `OwnedSemaphorePermit`.
@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Apr 7, 2024
@mox692 mox692 added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Apr 7, 2024
Copy link
Member

@mox692 mox692 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, such an api seems reasonable to me. I left a comment regarding the api.

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Darksonn commented Apr 8, 2024

I think split would be a better name.

@vvvviiv
Copy link
Contributor Author

vvvviiv commented Apr 9, 2024

I think split would be a better name.

Well, I chose the name split at first, but I changed my mind because of some reasons:

  1. split has several close meanings. Without reading the document, a user may think that split(n) means to split one SemaphorePermit into n SemaphorePermits. However, detach hints that we are separating some permits from one SemaphorePermit that has more permits. (I'm not a native speaker of English, maybe I misunderstood something?)
  2. The split method name is widely used. By the first impression of other split methods, it's easy to cause some misunderstanding like the example in 1.

But if you insist, the name split is also ok with me.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 9, 2024

You use the name detach to mean "detach from these permits", but to me, the name sounds like you are detaching the permits from the semaphore itself. So to me, detach sounds more like an alternate name for the forget method.

I suggested split because it's the opposite of merge.

So I'd prefer split or some third name if you have other suggestions.

@vvvviiv
Copy link
Contributor Author

vvvviiv commented Apr 9, 2024

It's finished.

@mox692 mox692 changed the title sync: add detach method to the permit types sync: add split method to the permit types Apr 10, 2024
Copy link
Member

@mox692 mox692 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@Darksonn Darksonn merged commit 224fea4 into tokio-rs:master Apr 10, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom-sync Run loom sync tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants