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

Simplify UnaryBlockingCall #225

Merged
merged 4 commits into from
Feb 23, 2024
Merged

Simplify UnaryBlockingCall #225

merged 4 commits into from
Feb 23, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented Feb 21, 2024

I found the implementation for this (with mutator methods and a kind of funky interaction between execute and cancel) a little confusing.

  • It's now just an interface, and the implementation is in the impl package (just like the stream interfaces).
  • The implementation is simpler and thread-safe. No more mutator methods needed.
  • It no longer supports calling it more than once (which it ostensibly did before, according to doc comments). This was incorrectly implemented anyway since, once invoked a subsequent time, it became impossible to cancel earlier invocations (and was not thread-safe). It is much easier to disallow multiple invocations than to try to make it properly support them. It's also much easier to reason about IMO.

@jhump jhump force-pushed the jh/fix-unary-call branch 2 times, most recently from 89894c1 to 6e86012 Compare February 22, 2024 00:44
@jhump jhump requested a review from pkwarren February 22, 2024 00:46
@jhump jhump force-pushed the jh/fix-unary-call branch 2 times, most recently from 9b849c8 to 4d5286b Compare February 23, 2024 16:59
- It's now just an interface, and the implementation is
  in the impl package (just like the stream interfaces).
- The implementation is simpler and thread-safe.
- No longer supports calling it more than once. This was
  incorrectly implemented before since it became impossible
  to cancel earlier invocations (and was not thread-safe).
  Much simpler to just not allow this than to fix it.
)
return@submit
}
callback.invoke(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this test be simplified to just call success on the callback without a delay? (So remove try/catch block above). Since nothing calls cancel, that shouldn't ever be invoked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good call.

val call = UnaryCall<Any> { callback ->
val future = executor.submit {
try {
Thread.sleep(1_000L)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be possible to coordinate these threads without relying on sleep. Maybe a countdown latch which is counted down here (before it just calls .wait() indefinitely). Then in the execution thread below instead of a sleep on line 108 we wait on the countdown latch to signal that this callable has started and will be interrupted when we call cancel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I also added another test for the case where call.cancel is invoked before call.execute.

@jhump jhump merged commit a8b1e58 into main Feb 23, 2024
7 checks passed
@jhump jhump deleted the jh/fix-unary-call branch February 23, 2024 19:46
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