Adds support for handle_continue/2 to gen_stage #264

hazardfn · 2020-10-08T18:03:02Z

{:continue, _term} instructions can now be returned as one would expect from gen_server.
:hibernate is now supported on init similar to gen_server.

Attempts to revive #227

hazardfn · 2020-10-08T18:14:09Z

The :continue tests will only work OTP 21+, not sure what the best strategy is for limiting these tests to those versions...

EDIT: Added a compile time conditional that checks the OTP major version, if there's a better way let me know! 😃

hazardfn · 2020-10-08T18:37:08Z

The failing test seems to be inherently flakey - I can reproduce on master. Not sure what's causing it but running mix test on a loop it occasionally shows up, can investigate separately:

Switched to branch 'master'
Your branch is up to date with 'origin/master'.

$ for i in 1 2 3 4 5; mix test; end
                                                                                                                                                              
Compiling 3 files (.ex)
................................................................................................................................................................................................

Finished in 6.0 seconds
192 tests, 0 failures

Randomized with seed 355674
................................................................................................................................................................................................

Finished in 6.0 seconds
192 tests, 0 failures

Randomized with seed 77758
.....................................................................................................

  1) test producer_consumer callbacks producer handle_cancel/3 on consumer down (GenStageTest)
     test/gen_stage_test.exs:1553
     Assertion failed, no matching message after 500ms
     The following variables were pinned:
       ref = #Reference<0.3941081809.3725066241.213827>
       consumer = #PID<0.589.0>
     Showing 2 of 2 messages in the mailbox
     code: assert_receive {:producer_consumer_cancelled, {^consumer, ^ref}, {:down, :killed}}
     mailbox:
       pattern: {:producer_consumer_cancelled, {^consumer, ^ref}, {:down, :killed}}
       value:   {:consumer_subscribed, {#PID<0.588.0>, #Reference<0.3941081809.3725066241.213827>}}

       pattern: {:producer_consumer_cancelled, {^consumer, ^ref}, {:down, :killed}}
       value:   {:producer_consumer_cancelled, {#PID<0.589.0>, #Reference<0.3941081809.3725066241.213827>}, {:down, :noproc}}
     stacktrace:
       test/gen_stage_test.exs:1560: (test)

..........................................................................................

Finished in 6.7 seconds
192 tests, 1 failure

Randomized with seed 791161
................................................................................................................................................................................................

Finished in 6.0 seconds
192 tests, 0 failures

Randomized with seed 199756
................................................................................................................................................................................................

Finished in 6.0 seconds
192 tests, 0 failures

Randomized with seed 937796

* {:continue, _term} instructions can now be returned as one would expect from gen_server. * :hibernate is now supported on init similar to gen_server.

hazardfn · 2020-10-19T06:58:37Z

Rebased for #265

hazardfn · 2021-01-11T14:00:10Z

@josevalim is there something I need to do to move this forward?

josevalim · 2021-01-11T14:02:12Z

Hi @hazardfn, apparently this fell under the cracks, sorry about that. I will try to save some time to review it this week but it may take a while because there are quite some factors to consider here. In any case, thanks for the PR and the ping!

acco · 2021-06-07T22:15:59Z

Would love this feature. Anything I can do to help test/QA/review?

whatyouhide · 2022-01-09T17:14:53Z

lib/gen_stage.ex

+            when new_state: term, event: term
+
+  @doc """
+  Invoked to handle `continue` instructions.


Suggested change

Invoked to handle `continue` instructions.

Invoked to handle `:continue` instructions.

whatyouhide · 2022-01-09T17:17:06Z

lib/gen_stage.ex

+  It is useful for performing work after initialization or for splitting the work
+  in a callback in multiple steps, updating the process state along the way.


I think this might be a bit misleading. I think the biggest use of this is being able to emit events and then do something right after, being sure that no messages, calls, or anything else will be "injected" in between. Maybe we can rephrase this?

My suggestion:

Suggested change

It is useful for performing work after initialization or for splitting the work

in a callback in multiple steps, updating the process state along the way.

This callback can be used to perform work right after emitting events from

other callbacks. The "continue mechanism" makes sure that no messages,

calls, casts, or anything else will be handled between a callback emitting

a `:continue` tuple and the `c:handle_continue/2` callback being invoked.

whatyouhide · 2022-01-09T17:17:24Z

lib/gen_stage.ex

+  Return values are the same as `c:handle_cast/2`.
+
+  This callback is optional. If one is not implemented, the server will fail
+  if a continue instruction is used.


Suggested change

if a continue instruction is used.

if a `:continue` instruction is used.

whatyouhide · 2022-01-09T17:18:36Z

lib/gen_stage.ex

+      {:producer, state, {:continue, _term} = continue} ->
+        init_producer(mod, [], state, continue)
+
+      {:producer, state, :hibernate} ->


I would force these return values to be {:producer, state, [], {:continue, term} | :hibernate} to simplify the API. Thoughts? Same for the cases below with producer_consumer and consumer.

I agree this incredibly long case statement is less than ideal but if I understand your suggestion correctly wouldn't this result in the GenStage not being initialized (validation/state setup) properly at all in the :hibernate case and in the {:continue, term} case initialization of the GenStage state would be forced upon the user...?

EDIT: Actually I think in the continue case it wouldn't work either because if I remember correctly user code only has access to the "inner" user state of the GenStage not the internals...

If you're OK with keeping the current logic as it is it might be cleaner to pull all this out into a pattern matched group of functions instead?

Right now you have a few extra cases because of the fact that you are allowing the third element of the tuple to be either the options, :hibernate, or {:continue, term}. What I am suggesting is that you don't allow it to be the options. The spec of the return would become something like {:producer, state :: term, options :: keyword, :hibernate | {:continue, term}}.

This way, you shave off a few case clauses by forcing users to use an empty list of options ([]) if they have no options to pass but still want to use :hibernate or {:continue, term}. Does it make more sense?

Right I see, thanks for the clarification! I'll look into these changes tonight 👍

hazardfn · 2022-01-10T21:20:04Z

Just checking in to say I won't have as much time as I thought tonight, will post here when I'm able to actually take a bite out of this.

isaacsanders · 2022-02-01T17:15:14Z

This would be incredible to have access to.

hazardfn · 2023-05-12T09:48:14Z

Closed in favour of #300.

Adds full support for handle_continue/2 to gen_stage

8f1e4ca

* {:continue, _term} instructions can now be returned as one would expect from gen_server. * :hibernate is now supported on init similar to gen_server.

whatyouhide reviewed Jan 9, 2022

View reviewed changes

whatyouhide mentioned this pull request Jan 9, 2022

Support handle_continue #227

Closed

maartenvanvliet mentioned this pull request May 11, 2023

Add support for handle_continue/2 callback #300

Open

hazardfn closed this May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for handle_continue/2 to gen_stage #264

Adds support for handle_continue/2 to gen_stage #264

hazardfn commented Oct 8, 2020 •

edited

Loading

hazardfn commented Oct 8, 2020 •

edited

Loading

hazardfn commented Oct 8, 2020 •

edited

Loading

hazardfn commented Oct 19, 2020

hazardfn commented Jan 11, 2021

josevalim commented Jan 11, 2021

acco commented Jun 7, 2021

whatyouhide Jan 9, 2022

whatyouhide Jan 9, 2022

whatyouhide Jan 9, 2022

whatyouhide Jan 9, 2022

hazardfn Jan 9, 2022 •

edited

Loading

hazardfn Jan 9, 2022

whatyouhide Jan 10, 2022

hazardfn Jan 10, 2022

hazardfn commented Jan 10, 2022

isaacsanders commented Feb 1, 2022

hazardfn commented May 12, 2023

	Invoked to handle `continue` instructions.
	Invoked to handle `:continue` instructions.

		It is useful for performing work after initialization or for splitting the work
		in a callback in multiple steps, updating the process state along the way.

-  It is useful for performing work after initialization or for splitting the work
-  in a callback in multiple steps, updating the process state along the way.
+  This callback can be used to perform work right after emitting events from
+  other callbacks. The "continue mechanism" makes sure that no messages,
+  calls, casts, or anything else will be handled between a callback emitting
+  a `:continue` tuple and the `c:handle_continue/2` callback being invoked.

	if a continue instruction is used.
	if a `:continue` instruction is used.

Adds support for handle_continue/2 to gen_stage #264

Adds support for handle_continue/2 to gen_stage #264

Conversation

hazardfn commented Oct 8, 2020 • edited Loading

hazardfn commented Oct 8, 2020 • edited Loading

hazardfn commented Oct 8, 2020 • edited Loading

hazardfn commented Oct 19, 2020

hazardfn commented Jan 11, 2021

josevalim commented Jan 11, 2021

acco commented Jun 7, 2021

whatyouhide Jan 9, 2022

Choose a reason for hiding this comment

whatyouhide Jan 9, 2022

Choose a reason for hiding this comment

whatyouhide Jan 9, 2022

Choose a reason for hiding this comment

whatyouhide Jan 9, 2022

Choose a reason for hiding this comment

hazardfn Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

hazardfn Jan 9, 2022

Choose a reason for hiding this comment

whatyouhide Jan 10, 2022

Choose a reason for hiding this comment

hazardfn Jan 10, 2022

Choose a reason for hiding this comment

hazardfn commented Jan 10, 2022

isaacsanders commented Feb 1, 2022

hazardfn commented May 12, 2023

hazardfn commented Oct 8, 2020 •

edited

Loading

hazardfn commented Oct 8, 2020 •

edited

Loading

hazardfn commented Oct 8, 2020 •

edited

Loading

hazardfn Jan 9, 2022 •

edited

Loading