Reimplement shaper as a token bucket. #1213

kzemek · 2017-03-10T15:01:31Z

The previous shaper implementation penalized users for sending
several small packets in burst, causing connection/reconnection
to be greatly delayed. To change this behaviour, this commit
reimplements the shaper as a token bucket:
https://en.wikipedia.org/wiki/Token_bucket .

The PR also fixes several issues that came to light with un-delaying packets:

Fixes/reduces severity of stanza racing to SM buffer / mod_offline when recipient "flickers" on and off
- enables mod_offline for all presets
- adds monitored_map module for recurring usecase of storing data bound to peer-process lifetime
Splits test for notifying unavailable pubsub subscribers and disables the part that never really worked
Ensures MUC room won't be routed to after its process stops

rslota · 2017-03-13T07:27:07Z

apps/ejabberd/src/shaper.erl

-    new1(Data).
+    case ejabberd_config:get_global_option({shaper, Name, global}) of
+        undefined -> #shaper{};
+        none -> #shaper{};


Is there any way that ejabberd_config:get_global_option({shaper, Name, global}) returns none? The old code handles that case but not explicitly and it feels very redundant.

none is explicit setting for no shaper; it's there in the configs, I guess there's some value in that (e.g. in case we would want to have a different default?)

mentels · 2017-03-13T11:04:09Z

apps/ejabberd/src/shaper.erl

-    #maxrate{maxrate = MaxRate,
-             lastrate = 0,
-             lasttime = now_to_usec(now())}.
+    TokenGrowth = round(Shaper#shaper.max_rate * (Now - Shaper#shaper.last_update) / Second),


IMO an expression like (Now - Shaper#shaper.last_update) / Second deserves its own function so that a code reader immediately knowns what it means.

Totally agree :)

gadgetci · 2017-03-20T13:34:22Z

apps/ejabberd/src/ejabberd_sm.erl

      LUser :: 'error' | ejabberd:luser() | tuple(),
      LServer :: 'error' | ejabberd:lserver() | tuple(),
-      LResource :: 'error' | ejabberd:lresource() | [byte()] | tuple().
+      LResource :: 'error' | ejabberd:lresource() | [byte()] | tuple(),
+      ReplacedSessionsPIDs :: ordsets:ordset(pid()).
 check_existing_resources(LUser, LServer, LResource) ->
    %% A connection exist with the same resource. We replace it:
    Sessions = ?SM_BACKEND:get_sessions(LUser, LServer, LResource),


According to Elvis:

Don't use macros (like SM_BACKEND on line 862) as module names.

gadgetci · 2017-03-20T13:34:23Z

apps/ejabberd/src/ejabberd_sm.erl

+                        false -> {true, SID}
+                    end
+                end,
+                ?SM_BACKEND:get_sessions(LUser, LServer)),


According to Elvis:

Don't use macros (like SM_BACKEND on line 884) as module names.

gadgetci · 2017-03-20T13:34:24Z

apps/ejabberd/src/mod_offline.erl

@@ -285,6 +290,10 @@ init([Host, AccessMaxOfflineMsgs]) ->
 %%                                      {stop, Reason, State}
 %% Description: Handling call messages
 %%--------------------------------------------------------------------
+handle_call({pop_offline_messages, LUser, LServer}, {Pid, _}, State) ->
+    Result = ?BACKEND:pop_messages(LUser, LServer),


According to Elvis:

Don't use macros (like BACKEND on line 294) as module names.

gadgetci · 2017-03-20T13:38:08Z

apps/ejabberd/src/ejabberd_c2s.erl

+handle_info(new_offline_messages, session_established,
+            #state{pres_last = Presence, pres_invis = Invisible} = StateData)
+  when Presence =/= undefined orelse Invisible ->
+    From = StateData#state.jid,


According to Compiler:

Warning: variable 'From' is unused

gadgetci · 2017-03-20T14:07:29Z

apps/ejabberd/src/ejabberd_sm.erl

      LUser :: 'error' | ejabberd:luser() | tuple(),
      LServer :: 'error' | ejabberd:lserver() | tuple(),
-      LResource :: 'error' | ejabberd:lresource() | [byte()] | tuple().
+      LResource :: 'error' | ejabberd:lresource() | [byte()] | tuple(),
+      ReplacedSessionsPIDs :: ordsets:ordset(pid()).
 check_existing_resources(LUser, LServer, LResource) ->
    %% A connection exist with the same resource. We replace it:
    Sessions = ?SM_BACKEND:get_sessions(LUser, LServer, LResource),


According to Elvis:

Don't use macros (like SM_BACKEND on line 864) as module names.

gadgetci · 2017-03-20T14:07:30Z

apps/ejabberd/src/ejabberd_sm.erl

+                        false -> {true, SID}
+                    end
+                end,
+                ?SM_BACKEND:get_sessions(LUser, LServer)),


According to Elvis:

Don't use macros (like SM_BACKEND on line 886) as module names.

The previous shaper implementation penalized users for sending several small packets in burst, causing connection/reconnection to be greatly delayed. To change this behaviour, this commit reimplements the shaper as a token bucket: https://en.wikipedia.org/wiki/Token_bucket .

monitored_map holds values associated with a PID. When the process dies and the map is notified (via message pass-through), entries associated with the process are removed from the map.

* Remove the ability to concurrently read and write messages to mod_offline backend, giving some needed guarantees for flushing messages from replaced C2S. * If any messages will arrive after offline storage was read, the running process will be notified of new messages.

Previous Erlang versions throw different errors on missing keys in map.

kzemek · 2017-03-22T08:40:17Z

http://tide.erlang-solutions.com/public/3fa54415-502d-42fb-9c7d-ddd81b571acb

michalwski · 2017-03-23T11:39:26Z

Thanks!

kzemek added the waiting-for-review label Mar 10, 2017

rslota reviewed Mar 13, 2017

View reviewed changes

mentels approved these changes Mar 13, 2017

View reviewed changes

kzemek force-pushed the reimplement_shaper branch from 0f0787e to 8a59854 Compare March 20, 2017 13:34

gadgetci reviewed Mar 20, 2017

View reviewed changes

kzemek force-pushed the reimplement_shaper branch from 8a59854 to 5b70305 Compare March 20, 2017 14:07

gadgetci reviewed Mar 20, 2017

View reviewed changes

kzemek force-pushed the reimplement_shaper branch from 5b70305 to 348a06b Compare March 20, 2017 14:33

kzemek added 7 commits March 20, 2017 16:07

Add monitored_map module.

b0f0e40

monitored_map holds values associated with a PID. When the process dies and the map is notified (via message pass-through), entries associated with the process are removed from the map.

Wait for replaced C2Ss to stop when starting a new C2S.

b021b2a

Split notify_only_available_users_test from pubsub tests.

f37f756

Enable mod_offline for all presets.

4804a88

Add 'none' shaper form for better fit in old code.

59854a5

kzemek force-pushed the reimplement_shaper branch from 348a06b to 59854a5 Compare March 20, 2017 15:07

kzemek added 2 commits March 20, 2017 17:33

Don't match specific exception for monitored_map tests.

492e1e4

Previous Erlang versions throw different errors on missing keys in map.

Remove MUC room from Mnesia during (not after) terminate.

3b6c29a

kzemek force-pushed the reimplement_shaper branch from 84f1d7d to 3b6c29a Compare March 21, 2017 09:16

kzemek added 2 commits March 21, 2017 12:01

Wait for close (instead of instant check) in SM session leak test.

b77f406

Remove a spurious variable binding.

381cf2c

michalwski removed the waiting-for-review label Mar 23, 2017

michalwski merged commit 524ae59 into master Mar 23, 2017

michalwski deleted the reimplement_shaper branch March 23, 2017 11:39

This was referenced Mar 24, 2017

Improve PubSub coverage + fixes #1236

Closed

[#435] fix race condition in mod_offline #447

Closed

MongooseIM 2.1.0beta1 #1244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement shaper as a token bucket. #1213

Reimplement shaper as a token bucket. #1213

kzemek commented Mar 10, 2017 •

edited

Loading

rslota Mar 13, 2017

kzemek Mar 20, 2017

mentels Mar 13, 2017

michalwski Mar 13, 2017

gadgetci Mar 20, 2017

gadgetci Mar 20, 2017

gadgetci Mar 20, 2017

gadgetci Mar 20, 2017

gadgetci Mar 20, 2017

gadgetci Mar 20, 2017

kzemek commented Mar 22, 2017

michalwski commented Mar 23, 2017

Reimplement shaper as a token bucket. #1213

Reimplement shaper as a token bucket. #1213

Conversation

kzemek commented Mar 10, 2017 • edited Loading

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

kzemek commented Mar 22, 2017

michalwski commented Mar 23, 2017

kzemek commented Mar 10, 2017 •

edited

Loading