Skip to content
This repository

Stream next reset inac timer #29

Closed
wants to merge 3 commits into from

2 participants

Filipe Manana Chandrashekhar Mullaparthi
Filipe Manana

Hi Chandru,

The following one line patch fixes the timeout issues I told you I have when using may workers with the {stream_to, {Pid(), once}} option on a local and fast network.

Let me know if you agree with it.

regards,
Filipe Manana

added some commits January 09, 2011
Filipe Manana Reset inactivity timeout when stream_next is invoked
This avoids plenty of connection inactivity timeouts. From a logical point of view,
the inactivity timeout should be reset not only only data is received from the socket
but also when the client asks for more data.
c98fcc8
Filipe Manana Don't trigger new inactivity timer when socket data is received and c…
…aller controls the socket

Like in synchronous programming, in makes sense to start an inactivity timer only when the caller
does a "recv" call and cancel the timer as soon as data is received from the socket.
20b7f66
Filipe Manana

No longer one line :)
Please see the 2 commit messages for an explanation.

regards

Chandrashekhar Mullaparthi

I'm not sure about this patch set Filipe. What happens if the calling process just dies and never asks for more? How will this connection get closed?

I suppose eventually the server will close the connection, but there is a corner case. If a firewall between the client and server drops its state table at the same time, we will never receive a TCP FIN for this socket, and we'll never close it down.

The other thing is set_inac_timer cancels the timer before setting it, so the call to cancel_timer is a bit redundant. Maybe the patch is just to add the {eat_message, timeout} to set_inac_timer?

Filipe Manana

Hi Chandru,

"I'm not sure about this patch set Filipe. What happens if the calling process just dies and never asks for more? How will this connection get closed?"

For my case it's not a problem, since the worker is linked to my process, once my process dies, the work dies and the socket is closed. If the worker is not linked to the user's process, then I guess it's the same issue like in most other languages/VMs.

My line of thought here is simpler to understand if we forget Erlang active sockets and say, imagine you're doing C or Java and want a timeout of 30secs:

1) Every we do a socket "recv" call, we start a timer and if within 30 secs we receive no data, we trigger a timeout error;

2) If during the "recv" call we receive data, we clear the timer we set, but we don't start a new timer. We only start a new timer when a "recv" call is done

Basically, for me ibrowse:stream_next/1 is like a "recv" call.

I agree that adding the {eat_message, timeout} to the cancel_timer call done by set_inac_timer is a good idea. But I don't think it solves the timeout issues I have in a local, fast LAN. I don't have this issue when not using the {stream_to, {pid(), once}} option (doesn't happen with {stream_to, pid()}.

Does my explanation makes sense? (I might be missing something)

thanks :)

Chandrashekhar Mullaparthi
Owner

Ok, I guess if the caller wants to bypass the load balancing mechanism, then it is their responsibility to clean up. I've integrated this patch. Thank you.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

Jan 09, 2011
Filipe Manana Reset inactivity timeout when stream_next is invoked
This avoids plenty of connection inactivity timeouts. From a logical point of view,
the inactivity timeout should be reset not only only data is received from the socket
but also when the client asks for more data.
c98fcc8
Jan 10, 2011
Filipe Manana Don't trigger new inactivity timer when socket data is received and c…
…aller controls the socket

Like in synchronous programming, in makes sense to start an inactivity timer only when the caller
does a "recv" call and cancel the timer as soon as data is received from the socket.
20b7f66
Filipe Manana Fix misspelled value 'no_reply' => 'noreply' 53753a2
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 8 additions and 3 deletions. Show diff stats Hide diff stats

  1. 11  src/ibrowse_http_client.erl
11  src/ibrowse_http_client.erl
@@ -188,7 +188,7 @@ handle_info({stream_next, Req_id}, #state{socket = Socket,
188 188
                                           cur_req = #request{req_id = Req_id}} = State) ->
189 189
     %% io:format("Client process set {active, once}~n", []),
190 190
     do_setopts(Socket, [{active, once}], State),
191  
-    {noreply, State};
  191
+    {noreply, set_inac_timer(State)};
192 192
 
193 193
 handle_info({stream_next, _Req_id}, State) ->
194 194
     _Cur_req_id = case State#state.cur_req of
@@ -330,8 +330,13 @@ handle_sock_data(Data, #state{status           = get_body,
330 330
                             active_once(State_1)
331 331
                     end,
332 332
                     State_2 = State_1#state{interim_reply_sent = false},
333  
-                    State_3 = set_inac_timer(State_2),
334  
-                    {noreply, State_3};
  333
+                    case Ccs of
  334
+                    true ->
  335
+                        cancel_timer(State_2#state.inactivity_timer_ref, {eat_message, timeout}),
  336
+                        {noreply, State_2#state{inactivity_timer_ref = undefined}};
  337
+                    _ ->
  338
+                        {noreply, set_inac_timer(State_2)}
  339
+                    end;
335 340
                 State_1 ->
336 341
                     active_once(State_1),
337 342
                     State_2 = set_inac_timer(State_1),
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.