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

Async primitives #85

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Async primitives #85

merged 3 commits into from
Sep 24, 2024

Conversation

m8nmueller
Copy link
Contributor

This PR contains primitives for synchronization and concurrency management:

  • Resource to wrap allocation/cleanup of (currently mostly computation) resources in different layers of safety
  • Semaphore, because it's useful
    Additionally, the specification of Async.await in light of locking Sources, that care about there result being used (like Semaphores which the consumer might want to reset on cancellation), was clarified and implemented. Now, cancellation will be ignored if it happens after a listener lock acquire that leads to completion. Still, cancellation can be requested synchronously without waiting.

Copy link
Contributor

@natsukagami natsukagami left a comment

Choose a reason for hiding this comment

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

Some notes ;D good work!

shared/src/main/scala/async/Resource.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/Async.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/Resource.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/Resource.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/Resource.scala Show resolved Hide resolved
shared/src/main/scala/async/Semaphore.scala Outdated Show resolved Hide resolved
* @param value
* the grant counter
*/
class Semaphore private (private var value: AtomicInteger) extends Async.Source[Unit]:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want this to be a Source I think it's better to make it a source with a release value (a Source[SemGuard] with a .release method?) or maybe a Resource[Unit]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really want it to be a Source. But that's the only thing one can await, so I do need a Source implementation somehow. I could add it as hidden inner object or make Semaphore a trait and have the hidden implementation implement the Source, but both add (allocation/method lookup) overhead.
The SemGuard idea looks nice, but then I would want the Semaphore again to implement it as well, just returning this from the Source.
Using a Semaphore as a Resource is a good idea, but maybe not as the default interface.. ?

shared/src/main/scala/async/futures.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/futures.scala Show resolved Hide resolved
Copy link
Contributor

@natsukagami natsukagami left a comment

Choose a reason for hiding this comment

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

LGTM! Although, I wouldn't like it if we squashed the whole thing saying "Async Primitives", so let's try to rebase and squash it into a few commits (introduing Resource, implement Semaphores, etc)

@m8nmueller
Copy link
Contributor Author

I squashed the commits now without changing the overall code diff before/after.

The tests failure we just had showed a minor mistake in the test conception. I hate using platform locks on gears futures.

Should all be solved now. Merge as you are :)

@m8nmueller m8nmueller merged commit 54bffdd into lampepfl:main Sep 24, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants