-
Notifications
You must be signed in to change notification settings - Fork 192
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
Add support for handle_continue/2
callback
#300
base: main
Are you sure you want to change the base?
Add support for handle_continue/2
callback
#300
Conversation
* {:continue, _term} instructions can now be returned as one would expect from gen_server. * :hibernate is now supported on init similar to gen_server.
4216261
to
1a0f4a6
Compare
lib/gen_stage.ex
Outdated
init_producer(mod, opts, state) | ||
init_producer(mod, opts, state, nil) | ||
|
||
{:producer, state, opts, continue_or_hibernate} when is_list(opts) -> |
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 about the naming of continue_or_hibernate
as it can also contain the timeout
. Including that as well seems verbose, is there a shorter term 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.
I'd call it additional_info
.
defp handle_gen_server_init_args(:hibernate, stage), do: {:ok, stage, :hibernate} | ||
|
||
defp handle_gen_server_init_args(timeout, stage) | ||
when (is_integer(timeout) and timeout >= 0) or timeout == :infinity, |
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.
Testing the timeout value here. Should this guard be placed elsewhere as well?
@josevalim could you have a look? |
@whatyouhide I know you reviewed the last revision - any chance you can take a look at this one? 🙏 |
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’m totally down for this, @josevalim?
lib/gen_stage.ex
Outdated
init_producer(mod, opts, state) | ||
init_producer(mod, opts, state, nil) | ||
|
||
{:producer, state, opts, continue_or_hibernate} when is_list(opts) -> |
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'd call it additional_info
.
handle_continue/2
callback
@josevalim @whatyouhide - Hi there! I was just looking for this feature to enable timer base scheduling in my GenStage producer. Is there a chance this could be approved soon? |
I need to sit down and review it. The issue is that we need to consider all possible execution scenarios for producers, consumers, and producer_consumer, and those are not trivial. So I need to be in the proper headspace and other tasks take priority at the moment. Sorry. |
I took the liberty to address the feedback for this PR #264 as it was open for more than a year. @hazardfn hope you are okay with this?
GenServers also support a timeout in the return value
{:ok, state, timeout | :hibernate | {:continue, term}}
which is currently not supported by GenStage. I've added this too.