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

Make HotSwap safe to concurrent access #3480

Merged
merged 5 commits into from
Apr 15, 2023

Conversation

armanbilge
Copy link
Member

Fixes #2050. Closes #3473.

The current resource in Hotswap is now guarded by a mutex, which must be acquired to swap or clear it. A new method get also provides safe access to the current resource, by acquiring the mutex while it is in use.

Comment on lines +67 to 73
* For safer access to the current resource see [[get]], which guarantees that it will not be
* released while it is being used.
*
* If [[swap]] is called after the lifetime of the [[Hotswap]] is over, it will raise an
* error, but will ensure that all resources are finalized before returning.
*/
def swap(next: Resource[F, R]): F[R]
Copy link
Member Author

Choose a reason for hiding this comment

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

This API is a bit unsafey since it directly returns the R but I think we'll just have to live with that.

Comment on lines +75 to +79
/**
* Gets the current resource, if it exists. The returned resource is guaranteed to be
* available for the duration of the returned resource.
*/
def get: Resource[F, Option[R]]
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a little bit annoying UX that this is an Option[R], alternatively we could raise an error. But the user can decide to do that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bikeshed the name. access a la HotswapRef ?

@armanbilge armanbilge marked this pull request as draft March 5, 2023 04:21
@armanbilge
Copy link
Member Author

Woops, I didn't think this through :) we need to make sure that multiple readers don't block each other.

h/t https://github.com/janstenpickle/hotswap-ref

armanbilge and others added 2 commits March 5, 2023 04:41
Co-authored-by: catostrophe <40268503+catostrophe@users.noreply.github.com>
Comment on lines +124 to +128
def shared: Resource[F, Unit] = semaphore.permit

def exclusive: Resource[F, Unit] =
Resource.makeFull[F, Unit](poll => poll(semaphore.acquireN(Long.MaxValue)))(_ =>
semaphore.releaseN(Long.MaxValue))
Copy link
Member Author

Choose a reason for hiding this comment

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

Stole this from here:
https://github.com/janstenpickle/hotswap-ref/blob/34781a0af15784981cd007b74f0f5c31836d73f3/modules/core/src/main/scala/io/janstenpickle/hotswapref/Lock.scala

It seems like a good addition to std on its own merits.

trait Lock[F[_]] {
  def shared: Resource[F, Unit]
  def exclusive: Resource[F, Unit]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Lock would probably be a good addition, but the tricky bit is that it isn't reentrant, so we have to be really careful.

@armanbilge armanbilge marked this pull request as ready for review March 5, 2023 04:47
Comment on lines +124 to +128
def shared: Resource[F, Unit] = semaphore.permit

def exclusive: Resource[F, Unit] =
Resource.makeFull[F, Unit](poll => poll(semaphore.acquireN(Long.MaxValue)))(_ =>
semaphore.releaseN(Long.MaxValue))
Copy link
Member

Choose a reason for hiding this comment

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

Lock would probably be a good addition, but the tricky bit is that it isn't reentrant, so we have to be really careful.

@djspiewak djspiewak merged commit 62c0f7f into typelevel:series/3.x Apr 15, 2023
@armanbilge armanbilge mentioned this pull request Apr 15, 2023
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.

Hotswap type parameter isn't really used Hotswap exposes problems with resource leaks on concurrent access
2 participants