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
Restore timeouts with wallclock time #140
Restore timeouts with wallclock time #140
Conversation
5c1a9a9
to
25d520d
Compare
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.
Great stuff. Dense, so it took a while to understand, but I think it makes sense.
Also somewhere centralized we should probably have a |
25d520d
to
441546b
Compare
Missed this in my revision pass yesterday! |
Didn't realize you were doing these changes in a fork. Sent you an invite for write access so you can do them directly in the repo in the future. |
441546b
to
7daab67
Compare
val timeoutJob = launch(Dispatchers.Default) { delay(timeout) } | ||
|
||
select { |
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.
Not sure why this garbage is passing spotless, but it is
cfc5abb
to
3ecc453
Compare
if (timeout != null) { | ||
withTurbineTimeout(timeout, block) | ||
} else { | ||
block() | ||
} |
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.
Why do this instead of passing the timeout into ChannelTurbine
?
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.
- Passing the timeout into
ChannelTurbine
wraps every singlereceive()
in awithContext
call, while this only requires one - Doing this means that the timeout is also applied to any other Turbines run within this block.
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 don't think either of those things apply here. The block
is only a call to flow.collect { channel.trySend(it) }
and never runs user code (unless they're using Turbine inside flow { }
). This coroutine's context isn't shared by the caller interacting with the Turbine
instance.
I'll write a quick test for it.
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.
Oof, you're right. We probably don't even need to be hooking up the timeout at all here: the timeout isn't even accessed in this context.
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.
Issue addressed; new tests in FlowTest
and FlowInScopeTest
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.
We probably just wrote the same tests. Signing off for a few hours.
3ecc453
to
7fba2d1
Compare
Addresses remarks on #130.