Expose `Semaphore::close` #3065

zaharidichev · 2020-10-28T14:51:49Z

Motivation

The need to expose Semaphore::close as explained in #3061.

Solution

Expose Semaphore::close

tokio/src/sync/semaphore.rs

carllerche · 2020-10-28T16:11:38Z

I believe that @darkson is correct. In order to expose close(), we would need to change acquire() to return Result.

If we want to make this change, we would need to make it in 1.0 as it is breaking.

zaharidichev · 2020-10-28T16:17:08Z

That makes sense. Should I close that for the time being then?

carllerche · 2020-11-10T17:55:18Z

I assigned this to the 1.0 milestone. We can leave it open for now.

taiki-e · 2020-12-10T10:29:11Z

triage: this is no longer blocked

LucioFranco · 2020-12-10T17:11:48Z

@carllerche @Darksonn can we get this reviewed? I don't have much context

LucioFranco · 2020-12-10T17:12:12Z

or figure out how to move forward here.

hawkw · 2020-12-10T17:42:38Z

@carllerche @Darksonn can we get this reviewed? I don't have much context

I'll take a look.

Darksonn · 2020-12-10T17:59:51Z

To move forward this needs to have the acquire methods return a result in case the semaphore has been closed. Or an alternative error-handling should be proposed.

hawkw

As @Darksonn pointed out in https://github.com/tokio-rs/tokio/pull/3065/files#r513557608, the Semaphore::acquire and Semaphore::acquire_owned methods currently unwrap the inner semaphore's result --- this will panic if we close the semaphore:

tokio/tokio/src/sync/semaphore.rs

Lines 98 to 105 in 917ce40

    
           /// Acquires permit from the semaphore. 
        
           pub async fn acquire(&self) -> SemaphorePermit<'_> { 
        
               self.ll_sem.acquire(1).await.unwrap(); 
        
               SemaphorePermit { 
        
                   sem: &self, 
        
                   permits: 1, 
        
               } 
        
           }

These need to be changed to also return Result, like

    /// Acquires permit from the semaphore.
    pub async fn acquire(&self) -> Result<SemaphorePermit<'_>, AcquireError> {
        self.ll_sem.acquire(1).await?;
        Ok(SemaphorePermit {
            sem: &self,
            permits: 1,
        })
    }

We'll also need to add a public error type for indicating that the semaphore is closed (could be a re-export of batch_semaphore::AcquireError). Similarly, we should update TryAcquireError to distinguish between cases where no permits are available and cases where the semaphore is closed (again, this could be a re-export of the inner batch_semaphore error type). Finally, we will want to add to the docs to explain the errors returned by these methods.

Then, I think this should be good to merge. Thanks!

carllerche · 2020-12-12T04:43:54Z

@zaharidichev we branched off 1.0. Are you able to complete this PR? Thanks!

zaharidichev · 2020-12-12T06:36:24Z

@carllerche Yes, I am able. Does it need to happen today?

carllerche · 2020-12-12T16:34:13Z

Not today. This week is fine!

Fixes: tokio-rs#3061 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

yjhmelody · 2020-12-14T17:01:17Z

tokio/src/sync/batch_semaphore.rs

    Closed,
+
+    /// The has no available permits.


The semaphore

hawkw

Overall, the change looks correct to me! I had some docs suggestions that it would be nice to address before merging — and let's also address Carl's comment about adding a usage example to Semaphore::close.

hawkw · 2020-12-14T17:58:59Z

tokio/src/sync/batch_semaphore.rs

@@ -41,15 +41,21 @@ struct Waitlist {
    closed: bool,
 }

-/// Error returned by `Semaphore::try_acquire`.
+/// Error returned from the `Semaphore::try_acquire` function.


This probably ought to be a link?

@hawkw Correct me if I am wrong, but it seems we cannot link to non public methods. Having this type be located in batch_semaphore and linking to the method in semaphore seems a bit odd. Am I missing something?

The type is exposed in the public API. Users will only ever see it via the public Semaphore type's methods; they shouldn't need to be aware of batch_semaphore, which is an implementation detail. I think it's correct for this to link to the public methods for readers of the documentation.

Makes sense. Thank you !

hawkw · 2020-12-14T17:59:10Z

tokio/src/sync/batch_semaphore.rs

    Closed,
+
+    /// The has no available permits.
    NoPermits,
 }
 /// Error returned by `Semaphore::acquire`.


and this one?

hawkw · 2020-12-14T18:03:25Z

tokio/src/sync/semaphore.rs

@@ -96,21 +88,21 @@ impl Semaphore {
    }

    /// Acquires permit from the semaphore.


It might be nice if this comment said something about errors, like

Suggested change

/// Acquires permit from the semaphore.

/// Acquires a permit from the semaphore.

///

/// If the semaphore has been closed, this returns an [`AcquireError`].

/// Otherwise, this returns a [`SemaphorePermit`] representing the

/// acquired permit.

or something?

hawkw · 2020-12-14T18:03:56Z

tokio/src/sync/semaphore.rs

            sem: &self,
            permits: 1,
-        }
+        })
    }

    /// Acquires `n` permits from the semaphore


and maybe this should say something like

Suggested change

/// Acquires `n` permits from the semaphore

/// Acquires `n` permits from the semaphore.

///

/// If the semaphore has been closed, this returns an [`AcquireError`].

/// Otherwise, this returns a [`SemaphorePermit`] representing the

/// acquired permits.

tokio/src/sync/semaphore.rs

hawkw · 2020-12-14T18:05:54Z

tokio/src/sync/batch_semaphore.rs

 #[derive(Debug)]
-pub(crate) enum TryAcquireError {
+pub enum TryAcquireError {
+    /// The semaphore has been closed and cannot issue new permits


Might be worth linking to the Semaphore::close method here?

Suggested change

/// The semaphore has been closed and cannot issue new permits

/// The semaphore has been [closed] and cannot issue new permits.

///

/// [closed]: crate::sync::Semaphore::close

hawkw · 2020-12-14T18:06:27Z

tokio/src/sync/batch_semaphore.rs

    NoPermits,
 }
 /// Error returned by `Semaphore::acquire`.
+///
+/// An `acquire` operation can only fail if the semaphore has been
+/// closed.


might be worth linking to the close method here too?

Suggested change

/// closed.

/// [closed].

///

/// [closed]: crate::sync::Semaphore::close

carllerche · 2020-12-15T05:49:49Z

@zaharidichev Thanks for updating the PR. I have no additional feedback. Once @hawkw's comments are addressed, I'm fine w/ merging this 👍.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev · 2020-12-15T14:27:35Z

I added some additional docs to other methods as well.

hawkw

Okay, the docs look great, thanks for adding examples. I'm going to merge this.

Thanks again for working on this, @zaharidichev!

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync labels Oct 28, 2020

Darksonn reviewed Oct 28, 2020

View reviewed changes

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved

carllerche reviewed Oct 28, 2020

View reviewed changes

tokio/src/sync/semaphore.rs Show resolved Hide resolved

carllerche added this to the v1.0 milestone Nov 10, 2020

taiki-e added the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Nov 20, 2020

taiki-e removed the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Dec 10, 2020

hawkw requested changes Dec 10, 2020

View reviewed changes

zaharidichev added 3 commits December 14, 2020 12:26

sync: expose Semaphore::close in public API

20f6815

Fixes: tokio-rs#3061 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

remove outdated comment

c603ca7

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

feedback

cbd629a

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/expose-semaphore-close branch from 917ce40 to cbd629a Compare December 14, 2020 13:26

remove private links in docs

625084c

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested a review from hawkw December 14, 2020 14:28

yjhmelody reviewed Dec 14, 2020

View reviewed changes

hawkw requested changes Dec 14, 2020

View reviewed changes

add docs and doc test

d184427

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested a review from hawkw December 15, 2020 14:27

hawkw approved these changes Dec 15, 2020

View reviewed changes

hawkw merged commit fcce78b into tokio-rs:master Dec 15, 2020

carllerche mentioned this pull request Dec 15, 2020

sync: consider exposing Semaphore::close in public API #3061

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `Semaphore::close` #3065

Expose `Semaphore::close` #3065

zaharidichev commented Oct 28, 2020

carllerche commented Oct 28, 2020

zaharidichev commented Oct 28, 2020

carllerche commented Nov 10, 2020

taiki-e commented Dec 10, 2020

LucioFranco commented Dec 10, 2020

LucioFranco commented Dec 10, 2020

hawkw commented Dec 10, 2020

Darksonn commented Dec 10, 2020

hawkw left a comment

carllerche commented Dec 12, 2020

zaharidichev commented Dec 12, 2020

carllerche commented Dec 12, 2020

yjhmelody Dec 14, 2020

hawkw left a comment

hawkw Dec 14, 2020

zaharidichev Dec 14, 2020

hawkw Dec 14, 2020

zaharidichev Dec 14, 2020

hawkw Dec 14, 2020

hawkw Dec 14, 2020

hawkw Dec 14, 2020

hawkw Dec 14, 2020

hawkw Dec 14, 2020

carllerche commented Dec 15, 2020

zaharidichev commented Dec 15, 2020

hawkw left a comment

	/// Acquires permit from the semaphore.
	pub async fn acquire(&self) -> SemaphorePermit<'_> {
	self.ll_sem.acquire(1).await.unwrap();
	SemaphorePermit {
	sem: &self,
	permits: 1,
	}
	}

		@@ -96,21 +88,21 @@ impl Semaphore {
		}

		/// Acquires permit from the semaphore.

-    /// Acquires permit from the semaphore.
+    /// Acquires a permit from the semaphore.
+    ///
+    /// If the semaphore has been closed, this returns an [`AcquireError`].
+    /// Otherwise, this returns a [`SemaphorePermit`] representing the
+    /// acquired permit.

-    /// The semaphore has been closed and cannot issue new permits
+    /// The semaphore has been [closed] and cannot issue new permits.
+    ///
+    /// [closed]: crate::sync::Semaphore::close

Expose Semaphore::close #3065

Expose Semaphore::close #3065

Conversation

zaharidichev commented Oct 28, 2020

Motivation

Solution

carllerche commented Oct 28, 2020

zaharidichev commented Oct 28, 2020

carllerche commented Nov 10, 2020

taiki-e commented Dec 10, 2020

LucioFranco commented Dec 10, 2020

LucioFranco commented Dec 10, 2020

hawkw commented Dec 10, 2020

Darksonn commented Dec 10, 2020

hawkw left a comment

Choose a reason for hiding this comment

carllerche commented Dec 12, 2020

zaharidichev commented Dec 12, 2020

carllerche commented Dec 12, 2020

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche commented Dec 15, 2020

zaharidichev commented Dec 15, 2020

hawkw left a comment

Choose a reason for hiding this comment

Expose `Semaphore::close` #3065

Expose `Semaphore::close` #3065