Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Add blocking checkout to quickcheck test and fix poolboy bug

The bug discovered was that blocking checkouts that had expired were not
being removed from the list of 'waiting' checkouts, so when a checkout
had timed out and a worker was subsequently checked in, the new worker
would be sent to the expired checkout request, not any current ones.
  • Loading branch information...
commit 6a53f06f8f09ae1022bc8bac6c2196688c03d8c8 1 parent 44a816e
Andrew Thompson authored January 20, 2012
83  src/poolboy.erl
@@ -35,7 +35,7 @@ checkout(Pool, Block) ->
35 35
 -spec checkout(Pool :: node(), Block :: boolean(), Timeout :: timeout())
36 36
     -> pid() | full.
37 37
 checkout(Pool, Block, Timeout) ->
38  
-    gen_fsm:sync_send_event(Pool, {checkout, Block}, Timeout).
  38
+    gen_fsm:sync_send_event(Pool, {checkout, Block, Timeout}, Timeout).
39 39
 
40 40
 -spec checkin(Pool :: node(), Worker :: pid()) -> ok.
41 41
 checkin(Pool, Worker) ->
@@ -80,7 +80,7 @@ ready({checkin, Pid}, State) ->
80 80
 ready(_Event, State) ->
81 81
     {next_state, ready, State}.
82 82
 
83  
-ready({checkout, Block}, {FromPid, _}=From, State) ->
  83
+ready({checkout, Block, Timeout}, {FromPid, _}=From, State) ->
84 84
     #state{workers = Workers,
85 85
            worker_sup = Sup,
86 86
            max_overflow = MaxOverflow,
@@ -102,7 +102,7 @@ ready({checkout, Block}, {FromPid, _}=From, State) ->
102 102
         {empty, Empty} ->
103 103
             Waiting = State#state.waiting,
104 104
             {next_state, full, State#state{workers=Empty,
105  
-                                           waiting=queue:in(From, Waiting)}}
  105
+                                           waiting=add_waiting(From, Timeout, Waiting)}}
106 106
     end;
107 107
 ready(_Event, _From, State) ->
108 108
     {reply, ok, ready, State}.
@@ -127,7 +127,7 @@ overflow({checkin, Pid}, State) ->
127 127
 overflow(_Event, State) ->
128 128
     {next_state, overflow, State}.
129 129
 
130  
-overflow({checkout, Block}, From, #state{overflow=Overflow,
  130
+overflow({checkout, Block, Timeout}, From, #state{overflow=Overflow,
131 131
                                          max_overflow=MaxOverflow
132 132
                                          }=State) when Overflow >= MaxOverflow ->
133 133
     case Block of
@@ -135,9 +135,9 @@ overflow({checkout, Block}, From, #state{overflow=Overflow,
135 135
             {reply, full, full, State};
136 136
         Block ->
137 137
             Waiting = State#state.waiting,
138  
-            {next_state, full, State#state{waiting=queue:in(From, Waiting)}}
  138
+            {next_state, full, State#state{waiting=add_waiting(From, Timeout, Waiting)}}
139 139
     end;
140  
-overflow({checkout, _Block}, {From, _}, State) ->
  140
+overflow({checkout, _Block, _Timeout}, {From, _}, State) ->
141 141
     #state{worker_sup=Sup, overflow=Overflow, worker_init=InitFun} = State,
142 142
     {Pid, Ref} = new_worker(Sup, From, InitFun),
143 143
     Monitors = [{Pid, Ref} | State#state.monitors],
@@ -154,12 +154,18 @@ full({checkin, Pid}, State) ->
154 154
         false -> State#state.monitors
155 155
     end,
156 156
     case queue:out(Waiting) of
157  
-        {{value, {FromPid, _}=From}, Left} ->
158  
-            Ref = erlang:monitor(process, FromPid),
159  
-            Monitors1 = [{Pid, Ref} | Monitors],
160  
-            gen_fsm:reply(From, Pid),
161  
-            {next_state, full, State#state{waiting=Left,
162  
-                                           monitors=Monitors1}};
  157
+        {{value, {{FromPid, _}=From, Timeout, StartTime}}, Left} ->
  158
+            case wait_valid(StartTime, Timeout) of
  159
+                true ->
  160
+                    Ref = erlang:monitor(process, FromPid),
  161
+                    Monitors1 = [{Pid, Ref} | Monitors],
  162
+                    gen_fsm:reply(From, Pid),
  163
+                    {next_state, full, State#state{waiting=Left,
  164
+                                                   monitors=Monitors1}};
  165
+                _ ->
  166
+                    %% replay this event with cleaned up waiting queue
  167
+                    full({checkin, Pid}, State#state{waiting=Left})
  168
+            end;
163 169
         {empty, Empty} when MaxOverflow < 1 ->
164 170
             Workers = queue:in(Pid, State#state.workers),
165 171
             {next_state, ready, State#state{workers=Workers, waiting=Empty,
@@ -173,10 +179,10 @@ full({checkin, Pid}, State) ->
173 179
 full(_Event, State) ->
174 180
     {next_state, full, State}.
175 181
 
176  
-full({checkout, false}, _From, State) ->
  182
+full({checkout, false, _Timeout}, _From, State) ->
177 183
     {reply, full, full, State};
178  
-full({checkout, _Block}, From, #state{waiting=Waiting}=State) ->
179  
-    {next_state, full, State#state{waiting=queue:in(From, Waiting)}};
  184
+full({checkout, _Block, Timeout}, From, #state{waiting=Waiting}=State) ->
  185
+    {next_state, full, State#state{waiting=add_waiting(From, Timeout, Waiting)}};
180 186
 full(_Event, _From, State) ->
181 187
     {reply, ok, full, State}.
182 188
 
@@ -232,12 +238,18 @@ handle_info({'EXIT', Pid, _}, StateName, State) ->
232 238
                                                overflow=Overflow-1}};
233 239
         full when MaxOverflow < 1 ->
234 240
             case queue:out(Waiting) of
235  
-              {{value, {FromPid, _}=From}, LeftWaiting} ->
236  
-                  MonitorRef = erlang:monitor(process, FromPid),
237  
-                  Monitors2 = [{FromPid, MonitorRef} | Monitors],
238  
-                  gen_fsm:reply(From, new_worker(Sup, InitFun)),
239  
-                  {next_state, full, State#state{waiting=LeftWaiting,
240  
-                                                 monitors=Monitors2}};
  241
+              {{value, {{FromPid, _}=From, Timeout, StartTime}}, LeftWaiting} ->
  242
+                  case wait_valid(StartTime, Timeout) of
  243
+                      true ->
  244
+                          MonitorRef = erlang:monitor(process, FromPid),
  245
+                          Monitors2 = [{FromPid, MonitorRef} | Monitors],
  246
+                          gen_fsm:reply(From, new_worker(Sup, InitFun)),
  247
+                          {next_state, full, State#state{waiting=LeftWaiting,
  248
+                                                         monitors=Monitors2}};
  249
+                      _ ->
  250
+                          %% replay it
  251
+                          handle_info({'EXIT', Pid, foo}, StateName, State#state{waiting=LeftWaiting})
  252
+                  end;
241 253
               {empty, Empty} ->
242 254
                   Workers2 = queue:in(new_worker(Sup, InitFun), State#state.workers),
243 255
                   {next_state, ready, State#state{monitors=Monitors,
@@ -246,13 +258,19 @@ handle_info({'EXIT', Pid, _}, StateName, State) ->
246 258
           end;
247 259
         full when Overflow =< MaxOverflow ->
248 260
             case queue:out(Waiting) of
249  
-              {{value, {FromPid, _}=From}, LeftWaiting} ->
250  
-                  MonitorRef = erlang:monitor(process, FromPid),
251  
-                  Monitors2 = [{FromPid, MonitorRef} | Monitors],
252  
-                  NewWorker = new_worker(Sup, InitFun),
253  
-                  gen_fsm:reply(From, NewWorker),
254  
-                  {next_state, full, State#state{waiting=LeftWaiting,
255  
-                                                 monitors=Monitors2}};
  261
+              {{value, {{FromPid, _}=From, Timeout, StartTime}}, LeftWaiting} ->
  262
+                  case wait_valid(StartTime, Timeout) of
  263
+                      true ->
  264
+                          MonitorRef = erlang:monitor(process, FromPid),
  265
+                          Monitors2 = [{FromPid, MonitorRef} | Monitors],
  266
+                          NewWorker = new_worker(Sup, InitFun),
  267
+                          gen_fsm:reply(From, NewWorker),
  268
+                          {next_state, full, State#state{waiting=LeftWaiting,
  269
+                                                         monitors=Monitors2}};
  270
+                      _ ->
  271
+                          %% replay it
  272
+                          handle_info({'EXIT', Pid, foo}, StateName, State#state{waiting=LeftWaiting})
  273
+                  end;
256 274
               {empty, Empty} ->
257 275
                   {next_state, overflow, State#state{monitors=Monitors,
258 276
                                                      overflow=Overflow-1,
@@ -294,3 +312,12 @@ prepopulate(0, _Sup, Workers, _InitFun) ->
294 312
     Workers;
295 313
 prepopulate(N, Sup, Workers, InitFun) ->
296 314
     prepopulate(N-1, Sup, queue:in(new_worker(Sup, InitFun), Workers), InitFun).
  315
+
  316
+add_waiting(From, Timeout, Queue) ->
  317
+    queue:in({From, Timeout, os:timestamp()}, Queue).
  318
+
  319
+wait_valid(infinity, _) ->
  320
+    true;
  321
+wait_valid(StartTime, Timeout) ->
  322
+    Waited = timer:now_diff(os:timestamp(), StartTime),
  323
+    (Waited div 1000) < Timeout.
23  test/poolboy_eqc.erl
@@ -31,6 +31,8 @@ command(S) ->
31 31
 		[{call, ?MODULE, start_poolboy, make_args(S, nat(), nat())} || S#state.pid == undefined] ++
32 32
 			[{call, ?MODULE, stop_poolboy, [S#state.pid]} || S#state.pid /= undefined] ++
33 33
 			[{call, ?MODULE, checkout_nonblock, [S#state.pid]} || S#state.pid /= undefined] ++
  34
+			%% checkout shrinks to checkout_nonblock so we can simplify counterexamples
  35
+			[{call, ?MODULE, ?SHRINK(checkout_block, [checkout_nonblock]), [S#state.pid]} || S#state.pid /= undefined] ++
34 36
 			[{call, ?MODULE, checkin, [S#state.pid, elements(S#state.checked_out)]} || S#state.pid /= undefined, S#state.checked_out /= []]
35 37
 	).
36 38
 
@@ -48,6 +50,9 @@ stop_poolboy(Pid) ->
48 50
 checkout_nonblock(Pool) ->
49 51
 	poolboy:checkout(Pool, false).
50 52
 
  53
+checkout_block(Pool) ->
  54
+	catch(poolboy:checkout(Pool, true, 100)).
  55
+
51 56
 checkin(Pool, Worker) ->
52 57
 	Res = poolboy:checkin(Pool, Worker),
53 58
 	gen_fsm:sync_send_all_state_event(Pool, get_avail_workers),
@@ -64,6 +69,13 @@ precondition(S, {call, _, checkin, [_Pool, Pid]}) ->
64 69
 precondition(_S,{call,_,_,_}) ->
65 70
 	true.
66 71
 
  72
+postcondition(S,{call,_,checkout_block,[_Pool]},R) ->
  73
+	case R of
  74
+		{'EXIT', {timeout, _}} ->
  75
+			length(S#state.checked_out) >= S#state.size + S#state.max_overflow;
  76
+		_ ->
  77
+			length(S#state.checked_out) < S#state.size + S#state.max_overflow
  78
+	end;
67 79
 postcondition(S,{call,_,checkout_nonblock,[_Pool]},R) ->
68 80
 	case R of
69 81
 		full ->
@@ -83,7 +95,16 @@ next_state(S,V,{call,_,start_poolboy, [Args]}) ->
83 95
 	};
84 96
 next_state(S,_V,{call,_,stop_poolboy, [_Args]}) ->
85 97
 	S#state{pid=undefined, checked_out=[]}; 
86  
-next_state(S,V,{call,_,checkout_nonblock, _}) ->
  98
+next_state(S,V,{call,_,checkout_block,_}) ->
  99
+	%% if the model says the checkout worked, store the result
  100
+	case checkout_ok(S) of
  101
+		false ->
  102
+			S;
  103
+		_ ->
  104
+			S#state{checked_out=S#state.checked_out++[V]}
  105
+	end;
  106
+next_state(S,V,{call,_,checkout_nonblock,_}) ->
  107
+	%% if the model says the checkout worked, store the result
87 108
 	case checkout_ok(S) of
88 109
 		false ->
89 110
 			S;

0 notes on commit 6a53f06

Please sign in to comment.
Something went wrong with that request. Please try again.