Add support for callback functions around worker process events #153

michalwski · 2018-10-09T17:50:24Z

This PR allows to set callback funs for following events in worker process:

on_init_started - initialization started (before module's init function is called
on_new_worker - when the module's init function returns with result {ok, State} or {ok, State, Timeout}
on_worker_dead - when worker process terminates

These callbacks can be used by worker_pool users in order to increase relevant metrics or add logging when needed.

michalwski · 2018-10-09T17:53:12Z

@elbrujohalcon when improving esl/MongooseIM to use worker_pool for every outgoing pool of connections (databases, external HTTP services) we come up with an idea of callbacks around some worker process events. Could you please let us know:

What do you think about the idea.
If the proposed approach is acceptable.

elbrujohalcon · 2018-10-10T11:23:22Z

Hi @michalwski. I like the concept in principle but I have a few concerns.

Crashes / Timeouts

As it is implemented now, callbacks my crash the process (we're not checking them for errors) and they can also block it (they are run sequentially within the wpool_process functions). I would like to have callback execution happening in an separate process.

Specs / Types

I would like to be more strict on what a callback function can be, for that I would prefer, if possible, to turn wpool_worker_callbacks (which I would rename wpool_process_callbacks) into a behavior with all callbacks in -optional_callbacks.

Multiplicity

It seems like you're only allowing one function per callback. In general, it might be nice to allow users to dynamically add/remove various functions to each callback.

Existing Functionality

Considering these previous items, my mind immediately jumps to Hey! Don't we have a generic way to implement callbacks anywhere? And I think, can't we implement this with gen_event instead of recreating that behavior. We could start a gen_event dispatcher for each pool and then provide helper functions for users to attach to them. It's fine to allow them to list the functions in the Options, too.

michalwski · 2018-10-10T11:37:53Z

Thanks for you reply @elbrujohalcon.

The gen_event approach looks interesting, at first I totally forgot about it. When I think about it now, I'm still not fully convinced. I think the callbacks functionality may not be used very frequently so maybe there is no need to start the dispatcher for every pool but rather on demand, when a callback is really needed.

I agree with all other points, this PR was just an initial work to show the idea. I'll continue working on it.

michalwski · 2018-10-10T13:51:08Z

After giving it more thought implementing these callbacks with gen_event may not be convenient for end users. They would have to implement their handlers (with all the gen_event callbacks). gen_event behavior is not commonly used so I'd rather hide it form the end user.

gen_event can still be used internally just to process the events in other process than the worker. There would be only one event_handler allowing to add/remove subscribers. These subscribers would implement the wpool_worker_callbacks behavior with optional callbacks.

What do you think about this @elbrujohalcon?

elbrujohalcon · 2018-10-10T13:53:45Z

Yeah, that's fine. Defining a new behavior wpool_process_callbacks and implementing it with a gen_event underneath is fine with me.

elbrujohalcon

It's good progress in general, but I think it needs some tweaking.

elbrujohalcon · 2018-10-11T10:26:33Z

src/wpool_pool.erl

+-spec add_callback_module(wpool:name(), module()) -> ok | {error, term()}.
+add_callback_module(Pool, Module) ->
+  EventManager = event_manager_name(Pool),
+  ok = gen_event:call(EventManager, wpool_process_callbacks, {add_callback, Module}).


Why instead of having a single event handler and adding callbacks to its list, don't we let gen_event take care of that and just add a new event handler per module?
This function would use gen_event:add_handler(EventManager, {wpool_process_callbacks, Module}, Module) and the one below gen_event:delete_handler(EventManager, {wpool_process_callbacks, Module}).

elbrujohalcon · 2018-10-11T10:28:05Z

src/wpool_process_callbacks.erl

+
+-spec handle_event({event(), [any()]}, state()) -> {ok, state()}.
+handle_event({Event, Args}, Callbacks) ->
+  [call(Callback, Event, Args) || Callback <- Callbacks],


If instead of managing a list of callbacks here, we let gen_event take care of that, this module (and in particular this function and the one below) will be simplified.

elbrujohalcon · 2018-10-11T10:29:01Z

src/wpool_pool.erl

+    , permanent
+    , brutal_kill
+    , worker
+    , [gen_event]


I think the right value for this element is dynamic

elbrujohalcon · 2018-10-11T10:30:47Z

src/wpool_pool.erl

@@ -340,6 +364,8 @@ process_sup_name(Sup) ->
  list_to_atom(?MODULE_STRING ++ [$-|atom_to_list(Sup)] ++ "-process-sup").
 queue_manager_name(Sup) ->
  list_to_atom(?MODULE_STRING ++ [$-|atom_to_list(Sup)] ++ "-queue-manager").
+event_manager_name(Sup) ->
+  list_to_atom(?MODULE_STRING ++ [$-|atom_to_list(Sup)] ++ "-event-handler").


should be -event-manager or -event-dispatcher, this one is not a handler.

src/wpool_process_callbacks.erl

elbrujohalcon · 2018-10-11T10:32:54Z

src/wpool_process_callbacks.erl

+
+-spec init([wpool:option()]) -> {ok, state()}.
+init(WPoolOpts) ->
+{ok, maybe_initial_callbacks(WPoolOpts)}.


indentation :)

Also, if instead of managing a list of callbacks here, we let gen_event take care of that, we won't need to initialize the list of callbacks here. We should do it in wpool_pool:start_link(…), probably after initializing the pool itself.

elbrujohalcon · 2018-10-11T10:35:47Z

src/wpool_process_callbacks.erl

+-export([notify/3]).
+-type state() :: ordset:ordset(module()).
+
+-type event() :: on_init_start | on_new_worker | on_worker_dead.


This module will define -callbacks for these 3 event types, right?

Also, to preserve the style of other behaviors, I would call them handle_worker_init, handle_worker_creation and handle_worker_death (or similar handle_<something> names).

elbrujohalcon · 2018-10-11T10:36:32Z

src/wpool_process_sup.erl

@@ -34,6 +34,7 @@ start_link(Parent, Name, Options) ->
 init({Name, Options}) ->
  Workers = proplists:get_value(workers, Options, 100),
  Strategy = proplists:get_value(strategy, Options, {one_for_one, 5, 60}),
+  maybe_add_event_handler(Options),


If instead of managing a list of callbacks ourselves, we let gen_event take care of that, this won't be needed.

This is still needed in order to add the initial callbacks passed in opts.

michalwski · 2018-10-11T15:35:06Z

Thanks for all the comments @elbrujohalcon. This PR was not ready yet but it's good to have a feedback as early as possible! I'll apply changes based on your review and let you know when I consider the work finished.

…nts-callback

michalwski · 2018-10-22T12:39:12Z

Triggering travis.

michalwski · 2018-10-23T07:46:09Z

@elbrujohalcon I consider this PR finished. Do you have any other comments?

codecov-io · 2018-10-23T07:54:20Z

Codecov Report

Merging #153 into rel-3.1 will decrease coverage by 0.67%.
The diff coverage is 89.58%.

@@             Coverage Diff             @@
##           rel-3.1     #153      +/-   ##
===========================================
- Coverage    95.73%   95.06%   -0.68%     
===========================================
  Files            9       10       +1     
  Lines          399      446      +47     
===========================================
+ Hits           382      424      +42     
- Misses          17       22       +5

Impacted Files	Coverage Δ
src/wpool_pool.erl	`94.87% <100%> (+0.42%)`	⬆️
src/wpool_process.erl	`100% <100%> (ø)`	⬆️
src/wpool_process_sup.erl	`100% <100%> (ø)`	⬆️
src/wpool_process_callbacks.erl	`79.16% <79.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e08b307...e6bd8a3. Read the comment docs.

elbrujohalcon

Looks really really good. Awesome job, @michalwski !! 💯
Just a few minor tweaks and we're done!

README.md

elbrujohalcon · 2018-10-23T15:11:03Z

src/wpool_process_callbacks.erl

+        , handle_call/2
+        , handle_info/2
+        , code_change/3
+        , terminate/2]).


Suggested change

, terminate/2]).

, terminate/2

]).

src/wpool_process_callbacks.erl

elbrujohalcon · 2018-10-23T15:14:13Z

src/wpool_process_sup.erl

+    undefined ->
+      ok;
+    EventMgr ->
+      [gen_event:add_handler(EventMgr, {wpool_process_callbacks, Module}, Module)


Since we don't care about the result of this list comprehension, why not using lists:foreach instead?

List comprehension feels more natural for me, that's why I used it here. There is no need to create a fun fro that, we can use the gen_event:add_handler directly here.
To optimize the code a bit I can return ok just after the list comprehension so the result will go to garbage sooner.
If you insist I can change to lists:foreach.

michalwski · 2018-10-24T08:12:12Z

@elbrujohalcon I think all the comments/suggestions were addressed in the latest commit.

elbrujohalcon

Just one final change, now that we moved to lists:foreach/2 (thanks for that, BTW)

src/wpool_process_sup.erl

elbrujohalcon · 2018-10-24T12:06:03Z

Note to self:

@elbrujohalcon don't forget to remove the optional callbacks from the gen_event implementation when replicating these changes on master.

elbrujohalcon · 2018-10-24T12:17:58Z

@michalwski you might want to consider adding something like

worker_pool/test/wpool_worker_SUITE.erl

Line 76 in 05a2411

-spec complete_coverage(config()) -> {comment, []}.

to appease @codecov-io 🤷‍♂️

Co-Authored-By: michalwski <michalwski@gmail.com>

elbrujohalcon · 2018-10-24T14:23:03Z

@michalwski it's approved. GREAT WORK! Merge it when you feel like it and I'll take care of propagating this changes to master later :)

michalwski · 2018-10-24T14:23:57Z

ou might want to consider adding something like ... to appease @codecov-io

I'm not sure about it. I'm fine with these 3 lines not covered. I can add it if you insist, though.

elbrujohalcon · 2018-10-24T14:25:01Z

I don't insist, we have #98 and most of them will go away with the usage of optional callbacks when migrating to master anyway…

michalwski · 2018-10-24T14:25:11Z

I'll merge it later today.

michalwski · 2018-10-24T14:27:23Z

@elbrujohalcon thanks for all your reviews and good suggestions!

michalwski added the WIP label Oct 9, 2018

add support for callback functions around worker initialisation

083c64c

michalwski force-pushed the rel-3.1-worker-events-callback branch from bdd5787 to 083c64c Compare October 10, 2018 07:35

add callback on worker's death

e0c1203

michalwski added 5 commits October 11, 2018 10:29

use gen_event to call callback modules

cd4d7c7

move callbacks tests to dedicated SUITE

bb27bb2

check if callback function is exported before calling it

5903fe2

allow to add and remove callbacks after pool is started

565131c

test that crashing callback does not affect others

bdd3975

elbrujohalcon requested changes Oct 11, 2018

View reviewed changes

michalwski added 6 commits October 22, 2018 09:40

let event_handler manage subscribed modules

065b817

Merge remote-tracking branch 'origin/rel-3.1' into rel-3.1-worker-eve…

4700ff9

…nts-callback

use better names for callback functions and manager

a806c53

use ktn:wait_for instead of hardcoded sleep times

93fd97b

start event manager for callbacks only if needed

bc1286b

update README.md

15633ad

michalwski force-pushed the rel-3.1-worker-events-callback branch from bf7562f to 37c7c38 Compare October 22, 2018 10:22

michalwski closed this Oct 22, 2018

michalwski reopened this Oct 22, 2018

michalwski closed this Oct 22, 2018

michalwski reopened this Oct 22, 2018

michalwski added 2 commits October 22, 2018 15:13

fix dialyzer and elvis warnings

03a7d8a

start the event manager only if enabled_callbacks is passed

dafe599

michalwski force-pushed the rel-3.1-worker-events-callback branch from 0e4b456 to dafe599 Compare October 22, 2018 13:14

michalwski closed this Oct 23, 2018

michalwski reopened this Oct 23, 2018

fix elvis issues found on travis

7f60837

michalwski added finished and removed WIP labels Oct 23, 2018

elbrujohalcon requested changes Oct 23, 2018

View reviewed changes

check if the callback module is laoded before adding a handler

1451eea

michalwski force-pushed the rel-3.1-worker-events-callback branch from 30b1246 to 1451eea Compare October 24, 2018 08:43

elbrujohalcon requested changes Oct 24, 2018

View reviewed changes

src/wpool_process_sup.erl Outdated Show resolved Hide resolved

Update src/wpool_process_sup.erl

e6bd8a3

Co-Authored-By: michalwski <michalwski@gmail.com>

elbrujohalcon approved these changes Oct 24, 2018

View reviewed changes

michalwski merged commit e94a682 into rel-3.1 Oct 24, 2018

elbrujohalcon deleted the rel-3.1-worker-events-callback branch October 5, 2021 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for callback functions around worker process events #153

Add support for callback functions around worker process events #153

michalwski commented Oct 9, 2018 •

edited

michalwski commented Oct 9, 2018

elbrujohalcon commented Oct 10, 2018

michalwski commented Oct 10, 2018

michalwski commented Oct 10, 2018

elbrujohalcon commented Oct 10, 2018

elbrujohalcon left a comment

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

elbrujohalcon Oct 11, 2018

michalwski Oct 22, 2018

michalwski Oct 22, 2018

michalwski commented Oct 11, 2018

michalwski commented Oct 22, 2018

michalwski commented Oct 23, 2018

codecov-io commented Oct 23, 2018 •

edited by codecov bot

elbrujohalcon left a comment

elbrujohalcon Oct 23, 2018

elbrujohalcon Oct 23, 2018

michalwski Oct 24, 2018

michalwski commented Oct 24, 2018

elbrujohalcon left a comment

elbrujohalcon commented Oct 24, 2018

elbrujohalcon commented Oct 24, 2018

elbrujohalcon commented Oct 24, 2018 •

edited

michalwski commented Oct 24, 2018

elbrujohalcon commented Oct 24, 2018

michalwski commented Oct 24, 2018

michalwski commented Oct 24, 2018

Add support for callback functions around worker process events #153

Add support for callback functions around worker process events #153

Conversation

michalwski commented Oct 9, 2018 • edited

michalwski commented Oct 9, 2018

elbrujohalcon commented Oct 10, 2018

Crashes / Timeouts

Specs / Types

Multiplicity

Existing Functionality

michalwski commented Oct 10, 2018

michalwski commented Oct 10, 2018

elbrujohalcon commented Oct 10, 2018

elbrujohalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Oct 11, 2018

michalwski commented Oct 22, 2018

michalwski commented Oct 23, 2018

codecov-io commented Oct 23, 2018 • edited by codecov bot

Codecov Report

elbrujohalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Oct 24, 2018

elbrujohalcon left a comment

Choose a reason for hiding this comment

elbrujohalcon commented Oct 24, 2018

elbrujohalcon commented Oct 24, 2018

elbrujohalcon commented Oct 24, 2018 • edited

michalwski commented Oct 24, 2018

elbrujohalcon commented Oct 24, 2018

michalwski commented Oct 24, 2018

michalwski commented Oct 24, 2018

michalwski commented Oct 9, 2018 •

edited

codecov-io commented Oct 23, 2018 •

edited by codecov bot

elbrujohalcon commented Oct 24, 2018 •

edited