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

Extend LeaseManager to support Leases as AutoCloseable resources #2319

Merged
merged 1 commit into from
Mar 13, 2022

Conversation

keeferrourke
Copy link
Collaborator

@keeferrourke keeferrourke commented Mar 11, 2022

Scratching an itch with the Lease interface.

Here's an implementation independent extension function to make leases auto-closeable.

Intention is to use it like:

@Inject lateinit val LeaseManager leaseManager

leaseManager.acquireOrNull("some-lease")?.use { lease ->
  /* Do critical stuff that needs the lease */
}

@@ -289,6 +294,28 @@ internal class ZkLeaseTest {
assertThat(acquireCalled.get()).isTrue()
}

@Test fun usableWithAutoCloseable() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dunno if this is the right place for this test.

@keeferrourke keeferrourke force-pushed the keefer.20220311.autoclosable-leases branch 2 times, most recently from 70c8764 to 28f1b27 Compare March 12, 2022 15:24
@keeferrourke keeferrourke changed the title Extend LeaseManager to suppose Leases as AutoCloseable resources Extend LeaseManager to support Leases as AutoCloseable resources Mar 12, 2022
@keeferrourke keeferrourke force-pushed the keefer.20220311.autoclosable-leases branch from 28f1b27 to 55ff29b Compare March 12, 2022 15:28
@keeferrourke keeferrourke merged commit 6b81f01 into master Mar 13, 2022
@keeferrourke keeferrourke deleted the keefer.20220311.autoclosable-leases branch March 13, 2022 22:39
Copy link
Collaborator

@aerb aerb left a comment

Choose a reason for hiding this comment

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

Like the direction! Just some thoughts on the API.

* }
* ```
*/
fun LeaseManager.acquireOrNull(name: String): AutoCloseableLease? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One potential downside of this is it's still possible to acquire and then forget to close a lease. I've seen the useX convention used in the kotlin stdlib that avoids this by just accepting a lambda directly. Example that jumps to mind is the useLines method.
Eg:

File("").useLines { lines -> println(lines) }

This may be a bit awkward her - I'm not sure what the behavior should be if you can't acquire ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You raise a good point, which is also still a problem with the prescribed usage without this extension.
Leases are held only for some time, so if a user forgets to close the lease, it will be let go eventually.

Something like leaseManager.useLease { lease -> ... } would be nicer from that perspective, however it would probably have to come with the stipulation that the lambda doesn't run if the lease can't be acquired. Unsure if that would be surprising. Perhaps leaseManager.maybeUseLease { lease -> ... }? Dunno! 🤷

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.

3 participants