-
Notifications
You must be signed in to change notification settings - Fork 521
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
Add Lock to std #3542
base: series/3.x
Are you sure you want to change the base?
Add Lock to std #3542
Conversation
abstract class Lock[F[_]] { | ||
def shared: Resource[F, Unit] | ||
def exclusive: Resource[F, Unit] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we should have an implementation here as well, apart from just HotSwap
. People are almost certainly going to try to do Lock[IO].flatMap(lock => ...)
and wonder why it doesn't work. :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to note the requested changes in the PR state :-)
@@ -51,7 +51,7 @@ import cats.syntax.all._ | |||
* | |||
* Ported from https://github.com/typelevel/fs2. | |||
*/ | |||
sealed trait Hotswap[F[_], R] { | |||
sealed trait Hotswap[F[_], R] extends Lock[F] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure Hotswap
should extend Lock
– it seems more like an implementation detail of Hotswap
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
abstract class Lock[F[_]] { | ||
def shared: Resource[F, Unit] | ||
def exclusive: Resource[F, Unit] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to consider adding some other APIs here. For example:
def tryShared: Resource[F, Boolean]
def tryExclusive: Resource[F, Boolean]
Also should check Chris's lib for inspiration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood ! I'm working on that !
def apply[F[_]: Concurrent](maxShared: Long): F[Lock[F]] = | ||
Semaphore[F](maxShared).map { semaphore => | ||
new Lock[F] { | ||
override def shared: Resource[F, Unit] = semaphore.permit | ||
override def exclusive: Resource[F, Unit] = | ||
Resource.makeFull((poll: Poll[F]) => poll(semaphore.acquireN(maxShared)))(_ => | ||
semaphore.releaseN(maxShared)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a test for this implementation. For example to verify that the exclusive lock is indeed exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely should be tested. It occurred to me the other day that I just didn't catch that; thanks for being on it, Arman. :-)
Not sure this is enough for folks that want traditional locking. Like its an approach, but locks are decently diverse. |
Resolves #3535