-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
89894c1
to
6e86012
Compare
9b849c8
to
4d5286b
Compare
- 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.
4d5286b
to
a885a81
Compare
… basic unit tests
a885a81
to
4af7f5d
Compare
) | ||
return@submit | ||
} | ||
callback.invoke( |
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.
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.
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.
Yep, good call.
val call = UnaryCall<Any> { callback -> | ||
val future = executor.submit { | ||
try { | ||
Thread.sleep(1_000L) |
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 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.
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.
Done. I also added another test for the case where call.cancel
is invoked before call.execute
.
I found the implementation for this (with mutator methods and a kind of funky interaction between execute and cancel) a little confusing.