Convert active IMAP connection to a pool

The `active` IMAP connection would become confused when used for concurrent requests, so it has been converted to a pool to allow for transactional request handling. All internal usage of raw IMAP connections has been converted to switchboard:with_imap/2.
Mainframe-Archive · Jul 5, 2016 · 804aa17 · 804aa17
1 parent 32279a2
commit 804aa17
Show file tree

Hide file tree

Showing 8 changed files with 54 additions and 47 deletions.
diff --git a/Makefile b/Makefile
@@ -21,7 +21,7 @@ ifeq ($(PROFILE), dev)
 	RELX_OPTS = --dev-mode
 endif
 
-DEPS	= lager gproc cowboy jsx
+DEPS	= lager gproc cowboy jsx poolboy
 
 include erlang.mk
 

diff --git a/src/imap.erl b/src/imap.erl
@@ -206,6 +206,9 @@ start(ConnSpec, Opts) ->
 %% @equiv start_link(ConnSpec, [])
 -spec start_link(connspec()) ->
     {ok, pid()} | _.
+%% Allow starting with Poolboy
+start_link([ConnSpec, Opts]) when is_tuple(ConnSpec), is_list(Opts) ->
+    start_link(ConnSpec, Opts);
 start_link(ConnSpec) ->
     start_link(ConnSpec, []).
 

diff --git a/src/switchboard.app.src b/src/switchboard.app.src
@@ -14,7 +14,8 @@
                   cowboy,
                   jsx,
                   ssl,
-                  eunit
+                  eunit,
+                  poolboy
                  ]},
   {mod, {switchboard_app, []}},
   {env, []}

diff --git a/src/switchboard.erl b/src/switchboard.erl
@@ -57,8 +57,8 @@
 
 -include("switchboard.hrl").
 
-% -type process() :: account | active.
--type keytype() :: account | active | idlers
+% -type process() :: account | pool.
+-type keytype() :: account | idlers | pool
                  | {idler | operator | idler_sup, imap:mailbox()}.
 -type pubsub_channel() :: new.
 
@@ -83,7 +83,7 @@ add(ConnSpec, Auth) ->
 %% Each mailbox provided will be monitored for new emails, and notifications
 %% of new email arrivals will be published to the <tt>new</tt> channel.
 %%
-%% Note: This will start an `active' IMAP connection which can be used for queries,
+%% Note: This will start a `pool' of IMAP connections which can be used for queries,
 %% as well as one `idler' IMAP connection per mailbox given. Some email providers
 %% <a href="https://support.google.com/mail/answer/97150?hl=en">limit</a> the number
 %% of open imap connections.
@@ -131,8 +131,8 @@ stop_mailbox_monitor(Account, Mailbox) ->
 %% wraps the term with gproc properties and a switchboard namespace.
 %%
 %% <dl>
-%%   <dt>`active'</dt>
-%%     <dd>The active IMAP client. While used internally by Switchboard,
+%%   <dt>`pool'</dt>
+%%     <dd>The pool of IMAP clients. While used internally by Switchboard,
 %%         this can also be used externally to make requests.
 %%     </dd>
 %%   <dt>`{idler, Mailbox}'</dt>
@@ -167,8 +167,8 @@ register_callback(Account, Type) ->
 
 %% @doc Returns the process of type for the provided account.
 %%
-%% For example, `where(<<"[email protected]">>, active)' would return
-%% the `active' IMAP client for `[email protected]'.
+%% For example, `where(<<"[email protected]">>, pool)' would return
+%% the `pool' supervisor of IMAP clients for `[email protected]'.
 %%
 %% @see key_for/2
 %% @end
@@ -198,19 +198,16 @@ await(Account, Type, Timeout) ->
     Pid.
 
 
-%% @todo implement a pool around active connections, checkout the active process
-%% from this.
 checkout(Account) ->
-    {Active, _} = gproc:await(key_for(Account, active), 5000),
+    Active = poolboy:checkout({via, gproc, key_for(Account, pool)}),
     {ok, Active}.
 
 
-%% @todo return a checked out IMAP connection.
-return(_Account, _IMAP) ->
-    ok.
+return(Account, IMAP) ->
+    ok = poolboy:checkin({via, gproc, key_for(Account, pool)}, IMAP).
 
 
-%% @todo rethrow errors instead of eating them?
+%% @doc Execute a function with an IMAP connection to the given account
 with_imap(Account, Fun) ->
     {ok, IMAP} = checkout(Account),
     Result = case catch Fun(IMAP) of
@@ -351,10 +348,10 @@ suite_test_() ->
      {foreach,
       fun() ->
               {ok, _} = switchboard:add(?DISPATCH_CONN_SPEC, ?DISPATCH_AUTH),
-              {_, _} = gproc:await(switchboard:key_for(?DISPATCH, active), 5000),
+              {_, _} = gproc:await(switchboard:key_for(?DISPATCH, pool), 5000),
               ?DISPATCH end,
       fun(Account) ->
-              IMAP = switchboard:where(Account, active),
+              IMAP = switchboard:where(Account, pool),
               ok = switchboard:stop(Account),
               switchboard_util:await_death(IMAP)
       end,
@@ -384,7 +381,7 @@ pubsub_assertions() ->
 %% @private
 where_assertions(Account) ->
     [?_assertMatch(P when is_pid(P), switchboard:await(Account, account)),
-     ?_assertMatch(P when is_pid(P), switchboard:await(Account, active))].
+     ?_assertMatch(P when is_pid(P), switchboard:await(Account, pool))].
 
 
 %% @private
@@ -406,7 +403,10 @@ monitor_assertions(Account) ->
 %% @private
 %% @doc Test that the imap server can be queried.
 query_assertions(Account) ->
-    Active = switchboard:await(Account, active),
-    [?_assertMatch({ok, _}, imap:call(Active, {select, <<"INBOX">>}))].
+    Res = switchboard:with_imap(Account,
+                                fun(IMAP) ->
+                                    imap:call(IMAP, {select, <<"INBOX">>})
+                                end),
+    [?_assertMatch({ok, _}, Res)].
 
 -endif.
diff --git a/src/switchboard_accounts.erl b/src/switchboard_accounts.erl
@@ -67,16 +67,12 @@ init({ConnSpec, Auth, Mailboxes}) ->
     true = gproc:reg(switchboard:key_for(Account, account)),
     RestartStrategy = one_for_all,
     MaxR = MaxT = 5,
-    ActiveChildSpec = {active,
-                       {imap, start_link,
-                        [ConnSpec,
-                         [{cmds, [{cmd, {call, {login, Auth}}}]},
-                          {post_init_callback,
-                           switchboard:register_callback(Account, active)}]]},
-                       permanent,
-                       5000,
-                       worker,
-                       [imap]},
+    ActiveChildSpec = poolboy:child_spec(
+                        Account,
+                        [{name,
+                          {via, gproc, switchboard:key_for(Account, pool)}},
+                         {worker_module, imap}, {size, 4}, {max_overflow, 5}],
+                        [ConnSpec, [{cmds, [{cmd, {call, {login, Auth}}}]}]]),
     IdlersChildSpec = {idlers,
                        {switchboard_idlers, start_link, [ConnSpec, Auth, Mailboxes]},
                        permanent,

diff --git a/src/switchboard_jmap.erl b/src/switchboard_jmap.erl
@@ -798,7 +798,7 @@ jmap_connect_assertions() ->
     Account = imap:auth_to_account(Auth),
     case lists:member(Account, switchboard:accounts()) of
         true ->
-            OldIMAP = switchboard:where(Account, active),
+            OldIMAP = switchboard:where(Account, pool),
             switchboard:stop(Account),
             switchboard_util:await_death(OldIMAP);
         false ->
@@ -809,7 +809,7 @@ jmap_connect_assertions() ->
                      {<<"port">>, Port},
                      {<<"auth">>, imap:auth_to_props(Auth)}],
                     <<"1">>}, #state{}),
-    {IMAP, _} = gproc:await(switchboard:key_for(Account, active), 5000),
+    {IMAP, _} = gproc:await(switchboard:key_for(Account, pool), 5000),
     [?_assertMatch({{<<"connected">>, _, <<"1">>},
                     #state{connspec=ConnSpec, account=Account}}, Connect),
      ?_assertEqual(ok, switchboard:stop(Account)),
@@ -833,7 +833,7 @@ imap_setup() ->
 %% @private
 %% @doc Exit the test imap connection.
 imap_teardown(#state{account=Account}) ->
-    IMAP = switchboard:where(Account, active),
+    IMAP = switchboard:where(Account, pool),
     switchboard:stop(Account),
     switchboard_util:await_death(IMAP).
 

diff --git a/src/switchboard_operator.erl b/src/switchboard_operator.erl
@@ -195,8 +195,10 @@ update_uid_internal(#state{account=Account,
                            mailbox=Mailbox,
                            last_uid=LastUid} = State,
                     Uid) when Uid > LastUid ->
-    {ok, {_, Emails}} = imap:call(switchboard:where(Account, active),
-                                  {uid, {fetch, {LastUid + 1, Uid}, <<"ALL">>}}),
+    {ok, {_, Emails}} = switchboard:with_imap(Account,
+                            fun(IMAP) ->
+                                imap:call(IMAP, {uid, {fetch, {LastUid + 1, Uid}, <<"ALL">>}})
+                            end),
     lists:foreach(
       fun({fetch, Data}) ->
               switchboard:publish(new, {new, {Account, Mailbox}, Data})
@@ -218,10 +220,11 @@ get_fetch_uid([_ | Rest]) ->
 -spec current_uid(binary(), imap:mailbox()) ->
     {ok, integer()}.
 current_uid(Account, Mailbox) ->
-    {Active, _} = gproc:await(switchboard:key_for(Account, active), 5000),
-    {ok, _} = imap:call(Active, {select, Mailbox}),
-    {ok, {_, Resps}} = imap:call(Active, {uid, {fetch, '*', <<"UID">>}}),
-    BinUid = get_fetch_uid(Resps),
+    BinUid = switchboard:with_imap(Account, fun(Active) ->
+        {ok, _} = imap:call(Active, {select, Mailbox}),
+        {ok, {_, Resps}} = imap:call(Active, {uid, {fetch, '*', <<"UID">>}}),
+        get_fetch_uid(Resps)
+    end),
     {ok, if is_integer(BinUid) -> BinUid;
             is_binary(BinUid)  -> binary_to_integer(BinUid)
          end}.

diff --git a/src/test/eunit/switchboard_accounts_tests.erl b/src/test/eunit/switchboard_accounts_tests.erl
@@ -6,7 +6,7 @@ accounts_test_() ->
      fun accounts_setup/0,
      fun accounts_teardown/1,
      [fun accounts_reg_asserts/1,
-      fun active_asserts/1,
+      fun pool_asserts/1,
       fun operator_asserts/1]}.
 
 -spec accounts_setup() ->
@@ -34,15 +34,19 @@ accounts_reg_asserts({{_ConnSpec, Auth}, Mailboxes, _}) ->
       || Type <- [idler, operator], Mailbox <- Mailboxes],
      [?_assertMatch({Pid, _} when is_pid(Pid),
                     gproc:await(switchboard:key_for(Account, Type)))
-      || Type <- [active, account]]].
+      || Type <- [pool, account]]].
 
--spec active_asserts({{imap:connspec(), imap:auth()}, [imap:mailbox()], pid()}) ->
+-spec pool_asserts({{imap:connspec(), imap:auth()}, [imap:mailbox()], pid()}) ->
     [any()].
-active_asserts({{_, Auth}, [Mailbox | _], _}) ->
+pool_asserts({{_, Auth}, [Mailbox | _], _}) ->
     Account = imap:auth_to_account(Auth),
-    {Active, _} = gproc:await(switchboard:key_for(Account, active)),
-    [?_assertMatch({ok, _}, imap:call(Active, {select, Mailbox})),
-     ?_assertMatch({ok, _}, imap:call(Active, {fetch, 1, [<<"UID">>]}))].
+    {Select, Fetch} = switchboard:with_imap(Account,
+                                            fun(IMAP) ->
+                                                {imap:call(IMAP, {select, Mailbox}),
+                                                 imap:call(IMAP, {fetch, 1, [<<"UID">>]})}
+                                            end),
+    [?_assertMatch({ok, _}, Select),
+     ?_assertMatch({ok, _}, Fetch)].
 
 %% @hidden Assert that the operator is behaving as expected.
 -spec operator_asserts({{imap:connspec(), imap:auth()}, [imap:mailbox()], pid()}) ->
-Original file line number
+Diff line change
@@ Expand Up / @@ -14,7 +14,8 @@ @@
                       cowboy,
                       jsx,
                       ssl,
-                      eunit
+                      eunit,
+                      poolboy
                      ]},
       {mod, {switchboard_app, []}},
       {env, []}
@@ Expand Down @@