-
Notifications
You must be signed in to change notification settings - Fork 529
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
Fix Hotswap#get
being blocked by Hotswap#swap
#3800
Fix Hotswap#get
being blocked by Hotswap#swap
#3800
Conversation
…ther than the entire lock and swapFinalizer call
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.
Can we add another test that swap
is cancelable if while waiting for a get
to release the old resource and also that the finalizer is run in this case?
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.
Tests are looking good, sorry just some style nits 😅
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
Hotswap#get
being blocked by Hotswap#swap
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.
Nice catch and excellent work!
Thanks! Didn't want this PR to get lost to the sands of time. Thank you for those tweaks! That use of |
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. I've left a comment, but it's really not that important.
Co-authored-by: Daniel Urban <urban.dani@gmail.com>
854e2b8
Hotswap#swap
currently acquires all permits for its Semaphore during allocation of thenext
Resource, blockingHotswap#get
until the new Resource has been instantiated. In many use cases this will not be perceptible but if the Resource has a long initialization time, any calls toget
will block until the new Resource is ready... effectively undermining the very premise of Hotswap.