dabeaz / curio

Good Curio!
Other
4.02k stars 241 forks source link

Expose value of semaphore and allow resizing it #245

Closed Fuyukai closed 6 years ago

Fuyukai commented 6 years ago

I'm writing something that uses semaphores for controlling access to a resource, and it'd be nice if I could see the value for it and the ability to resize it releasing waiting tasks as appropriate.

dabeaz commented 6 years ago

I could certainly expose the value with a property perhaps. What do you have in mind for resizing it beyond using the acquire() and release() methods on it?

Fuyukai commented 6 years ago

Trying to do excessive release() on BoundedSemaphore would break - I want the bound to be there but also to be able to potentially increase it.

Additionally, resizing could possibly have different semantics when resizing it to be smaller; would it block there too or just stop any new tasks from acquiring the semaphore until it goes above the high water mark again? I would've PR'd I didn't know how to figure this one out so it stopped me.

njsmith commented 6 years ago

trio.CapacityLimiter is a semaphore with this feature, might be useful for reference:

https://trio.readthedocs.io/en/latest/reference-core.html#trio.CapacityLimiter https://trio.readthedocs.io/en/latest/reference-core.html#trio.CapacityLimiter.total_tokens

I implemented it because I wanted a non-blocking way to adjust the size of trio's "thread pool".

dabeaz commented 6 years ago

I might be missing something, but wouldn't a normal unbounded Semaphore work? If one wanted to increase capacity, call release() on it a bunch of times. I probably am missing something.... ;-)

njsmith commented 6 years ago

There are two things that CapacityLimiter does that are (slightly) more clever than a simple unbounded semaphore:

Whether that's something you want I can't say, but that's the difference.

Fuyukai commented 6 years ago

A better way to say what I want is a BoundedSemaphore where the value and bound are both exposed, and the bound can be changed upwards easily. When I made the issue I honestly hadn't thought of a for x in range(new_bound): await sema.release() for resizing too.

dabeaz commented 6 years ago

I've exposed the value and bound attributes of Semaphore as read-only properties, but I'd need to think about any kind of resizing. This is not a typical semaphore feature and it's complicated by the fact that knowing the "value" of the semaphore doesn't really tell you anything about how many tasks might be actively using it for something.

I'd suspect that a Semaphore might only be a building block for creating something for generally useful for this (such as a CapacityLimiter object).

Fuyukai commented 6 years ago

After rethinking some of the API design I want, just being able to see the values fits my purposes fine.