Repository navigation
socket: distinguish eagain from closed, and retry transparently via write-select #2345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-0.7
Are you sure you want to change the base?
Changes from all commits
bc89964
5ed6492
fe3b4fd
daaad3b
cd0dd33
5daaee1
c34bb11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -263,7 +263,13 @@ handle_call({send, Packet}, From, State) -> | |
| {send, Packet}, From, State | ||
| ]), | ||
| ?LOG_INFO("Sending packet"), | ||
| {reply, socket:send(State#state.socket, Packet), State}; | ||
| Reply = | ||
| case socket:send(State#state.socket, Packet) of | ||
| % gen_tcp:send/2 does not report the data that was not sent | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's avoid narration comment, such this one. |
||
| {error, {Reason, _RestData}} -> {error, Reason}; | ||
| Result -> Result | ||
| end, | ||
| {reply, Reply, State}; | ||
| handle_call({recv, Length, Timeout}, From, State) -> | ||
| ?LOG_DEBUG("handle_call [~p], ~p, ~p]", [ | ||
| {recv, Length, Timeout}, From, State | ||
|
|
@@ -379,18 +385,18 @@ handle_info({'$socket', Socket, abort, {Ref, closed}}, State) -> | |
| case maps:get(Ref, State#state.pending_selects, undefined) of | ||
| undefined -> | ||
| ?LOG_WARNING("Unable to find select ref ~p in pending selects", [Ref]), | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| {noreply, State}; | ||
| {accept, From, _AcceptingProc, _Timeout} -> | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| gen_server:reply(From, {error, closed}), | ||
| {noreply, State}; | ||
| active -> | ||
| WrappedSocket = {?GEN_TCP_MONIKER, self(), ?MODULE}, | ||
| State#state.controlling_process ! {tcp_closed, WrappedSocket}, | ||
| {noreply, State}; | ||
| {passive, From, _Length, _Timeout} -> | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| gen_server:reply(From, {error, closed}), | ||
| {noreply, State#state{ | ||
| pending_selects = maps:remove(Ref, State#state.pending_selects) | ||
|
|
@@ -465,7 +471,7 @@ handle_accept(State, From, AcceptingProc) -> | |
| {error, _Reason} = Error -> | ||
| ?LOG_ERROR("Failed to accept connection: ~p", [Error]), | ||
| %% CAUTION: internal API | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| gen_server:reply(From, Error), | ||
| State | ||
| end. | ||
|
|
@@ -510,15 +516,15 @@ handle_active_recv(State) -> | |
| {error, closed} -> | ||
| ?LOG_INFO("Socket was closed by peer"), | ||
| %% CAUTION: internal API | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| %% peer has closed the connection | ||
| WrappedSocket = {?GEN_TCP_MONIKER, self(), ?MODULE}, | ||
| State#state.controlling_process ! {tcp_closed, WrappedSocket}, | ||
| State; | ||
| {error, _} = Error -> | ||
| ?LOG_ERROR("Error on receive on pending select Error=~p", [Error]), | ||
| %% CAUTION: internal API | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| %% TODO is this right? I don't think active receivers would know | ||
| %% what to do with this message. Maybe log it instead? | ||
| State#state.controlling_process ! {tcp_error, WrappedSocket, Error}, | ||
|
|
@@ -547,7 +553,7 @@ handle_passive_recv(State, From, Length, _Timeout) -> | |
| State; | ||
| {error, _} = Error -> | ||
| ?LOG_ERROR("Error on receive from socket:nif_recv Error=~p", [Error]), | ||
| socket:nif_select_stop(Socket), | ||
| socket:nif_select_read_stop(Socket), | ||
| ?LOG_INFO("unable to receive on pending select~n"), | ||
| gen_server:reply(From, Error), | ||
| State | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,10 +46,12 @@ | |
| %% internal nifs | ||
| -export([ | ||
| nif_select_read/2, | ||
| nif_select_write/2, | ||
| nif_accept/1, | ||
| nif_recv/2, | ||
| nif_recvfrom/2, | ||
| nif_select_stop/1, | ||
| nif_select_read_stop/1, | ||
| nif_send/2, | ||
| nif_sendto/3 | ||
| ]). | ||
|
|
@@ -269,7 +271,7 @@ accept0(Socket, Timeout) -> | |
| {'$socket', Socket, select, Ref} -> | ||
| case ?MODULE:nif_accept(Socket) of | ||
| {error, _} = E -> | ||
| ?MODULE:nif_select_stop(Socket), | ||
| ?MODULE:nif_select_read_stop(Socket), | ||
| E; | ||
| {ok, _Socket} = Reply -> | ||
| Reply | ||
|
|
@@ -382,7 +384,7 @@ recv0(Socket, Length, Timeout) -> | |
| {'$socket', Socket, select, Ref} -> | ||
| case ?MODULE:nif_recv(Socket, Length) of | ||
| {error, _} = E -> | ||
| ?MODULE:nif_select_stop(Socket), | ||
| ?MODULE:nif_select_read_stop(Socket), | ||
| E; | ||
| {ok, Data} -> | ||
| {ok, Data} | ||
|
|
@@ -433,7 +435,7 @@ recv0_r(Socket, Length, Timeout, EndQuery, Acc) -> | |
| {'$socket', Socket, select, Ref} -> | ||
| case ?MODULE:nif_recv(Socket, Length) of | ||
| {error, _} = E -> | ||
| ?MODULE:nif_select_stop(Socket), | ||
| ?MODULE:nif_select_read_stop(Socket), | ||
| E; | ||
| {ok, Data} -> | ||
| NewAcc = [Data | Acc], | ||
|
|
@@ -537,7 +539,7 @@ recvfrom0(Socket, Length, Timeout) -> | |
| {'$socket', Socket, select, Ref} -> | ||
| case ?MODULE:nif_recvfrom(Socket, Length) of | ||
| {error, _} = E -> | ||
| ?MODULE:nif_select_stop(Socket), | ||
| ?MODULE:nif_select_read_stop(Socket), | ||
| E; | ||
| {ok, {_Address, _Data}} = Reply -> | ||
| Reply | ||
|
|
@@ -571,49 +573,61 @@ recvfrom0_nowait(Socket, Length, Ref) -> | |
| %%----------------------------------------------------------------------------- | ||
| %% @param Socket the socket | ||
| %% @param Data the data to send | ||
| %% @returns `{ok, Rest}' if successful; `{error, Reason}', otherwise. | ||
| %% @returns `ok' if successful; `{error, Reason}', otherwise, or | ||
| %% `{error, {Reason, RestData}}' if an error occurred after some of | ||
| %% the data was already sent, `RestData' being the data that was not | ||
| %% sent. | ||
| %% @doc Send data on the specified socket. | ||
| %% | ||
| %% Note that this function will block until data is sent | ||
| %% on the socket. The data may not have been received by the | ||
| %% intended recipient, and the data may not even have been sent | ||
| %% over the network. | ||
| %% | ||
| %% Partial sends and transient send backpressure (e.g. a full TCP | ||
| %% send buffer) are retried internally, waiting for the socket to | ||
| %% become writable again, until all data has been accepted by the | ||
| %% stack or an error occurs. Callers therefore do not need to | ||
| %% implement their own retry logic for `eagain'-like conditions. | ||
| %% | ||
| %% Example: | ||
| %% | ||
| %% `ok = socket:send(ConnectedSocket, Data)' | ||
| %% @end | ||
| %%----------------------------------------------------------------------------- | ||
| -spec send(Socket :: socket(), Data :: iodata()) -> | ||
| ok | {ok, Rest :: binary()} | {error, Reason :: term()}. | ||
| ok | {error, Reason :: term()} | {error, {Reason :: term(), RestData :: binary()}}. | ||
| send(Socket, Data) when is_binary(Data) -> | ||
| ?MODULE:nif_send(Socket, Data); | ||
| send_all(Socket, Data); | ||
| send(Socket, Data) -> | ||
| ?MODULE:nif_send(Socket, erlang:iolist_to_binary(Data)). | ||
| send_all(Socket, erlang:iolist_to_binary(Data)). | ||
|
|
||
| %%----------------------------------------------------------------------------- | ||
| %% @param Socket the socket | ||
| %% @param Data the data to send | ||
| %% @param Dest the destination to which to send the data | ||
| %% @returns `{ok, Rest}' if successful; `{error, Reason}', otherwise. | ||
| %% @returns `ok' if successful; `{error, Reason}', otherwise. | ||
| %% @doc Send data on the specified socket to the specified destination. | ||
| %% | ||
| %% Note that this function will block until data is sent | ||
| %% on the socket. The data may not have been received by the | ||
| %% intended recipient, and the data may not even have been sent | ||
| %% over the network. | ||
| %% | ||
| %% Transient send backpressure is retried internally, waiting for | ||
| %% the socket to become writable again, as with `send/2'. | ||
| %% | ||
| %% Example: | ||
| %% | ||
| %% `ok = socket:sendto(ConnectedSocket, Data, Dest)' | ||
| %% @end | ||
| %%----------------------------------------------------------------------------- | ||
| -spec sendto(Socket :: socket(), Data :: iodata(), Dest :: sockaddr()) -> | ||
| ok | {ok, Rest :: binary()} | {error, Reason :: term()}. | ||
| ok | {error, Reason :: term()}. | ||
| sendto(Socket, Data, Dest) when is_binary(Data) -> | ||
| ?MODULE:nif_sendto(Socket, Data, Dest); | ||
| sendto_all(Socket, Data, Dest); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On BSD sockets, sendto/3 now waits on nif_select_write when sendto(2) returns EAGAIN (sendto_all, line 799), where before the PR the same condition surfaced as an immediate {error, closed}. gen_udp_socket:handle_call({sendto, ...}) at gen_udp_socket.erl:178 performs the send inside the gen_server, and send/4 calls it with an infinite timeout, so the server, and with it every other operation on that gen_udp socket, now waits until the socket is writable. OTP's gen_udp_socket sends from the calling process instead, so only the sender blocks there. Let's handle it: for example a nowait send with the pending datagram kept in the gen_server state, as suggested for gen_tcp_socket. The lwIP UDP path is unaffected, as it never reports would-block. |
||
| sendto(Socket, Data, Dest) -> | ||
| ?MODULE:nif_sendto(Socket, erlang:iolist_to_binary(Data), Dest). | ||
| sendto_all(Socket, erlang:iolist_to_binary(Data), Dest). | ||
|
|
||
| %%----------------------------------------------------------------------------- | ||
| %% @param Socket the socket | ||
|
|
@@ -708,6 +722,10 @@ shutdown(_Socket, _How) -> | |
| nif_select_read(_Socket, _Ref) -> | ||
| erlang:nif_error(undefined). | ||
|
|
||
| %% @private | ||
| nif_select_write(_Socket, _Ref) -> | ||
| erlang:nif_error(undefined). | ||
|
|
||
| %% @private | ||
| nif_accept(_Socket) -> | ||
| erlang:nif_error(undefined). | ||
|
|
@@ -724,10 +742,83 @@ nif_recvfrom(_Socket, _Length) -> | |
| nif_select_stop(_Socket) -> | ||
| erlang:nif_error(undefined). | ||
|
|
||
| %% @private | ||
| %% Like nif_select_stop/1, but if another process is selecting for write on the | ||
| %% socket, only the read direction is stopped. | ||
| nif_select_read_stop(_Socket) -> | ||
| erlang:nif_error(undefined). | ||
|
|
||
| %% @private | ||
| nif_send(_Socket, _Data) -> | ||
| erlang:nif_error(undefined). | ||
|
|
||
| %% @private | ||
| nif_sendto(_Socket, _Data, _Dest) -> | ||
| erlang:nif_error(undefined). | ||
|
|
||
| %% @private | ||
| %% Send all of Data on Socket, retrying partial sends and transient send | ||
| %% backpressure ({error, eagain}) by waiting for the socket to become | ||
| %% writable again, until all data has been accepted or a real error occurs. | ||
| %% As in OTP, an error that follows a partial send is reported as | ||
| %% {error, {Reason, RestData}}, and as {error, Reason} if nothing was accepted. | ||
| send_all(Socket, Data) -> | ||
| send_all(Socket, Data, false). | ||
|
|
||
| %% @private | ||
| send_all(Socket, Data, Partial) -> | ||
| case ?MODULE:nif_send(Socket, Data) of | ||
| ok -> | ||
| ok; | ||
| {ok, Rest} -> | ||
|
harmon25 marked this conversation as resolved.
Resolved
|
||
| send_all(Socket, Rest, true); | ||
| {error, eagain} -> | ||
| case wait_write(Socket) of | ||
| ok -> | ||
| send_all(Socket, Data, Partial); | ||
| {error, Reason} -> | ||
| send_error(Reason, Data, Partial) | ||
| end; | ||
| {error, Reason} -> | ||
| send_error(Reason, Data, Partial) | ||
| end. | ||
|
|
||
| %% @private | ||
| send_error(Reason, RestData, true) -> | ||
| {error, {Reason, RestData}}; | ||
| send_error(Reason, _RestData, false) -> | ||
| {error, Reason}. | ||
|
|
||
| %% @private | ||
| sendto_all(Socket, Data, Dest) -> | ||
| case ?MODULE:nif_sendto(Socket, Data, Dest) of | ||
| ok -> | ||
| ok; | ||
| {ok, Rest} -> | ||
| sendto_all(Socket, Rest, Dest); | ||
| {error, eagain} -> | ||
| case wait_write(Socket) of | ||
| ok -> | ||
| sendto_all(Socket, Data, Dest); | ||
| {error, _Reason} = Error -> | ||
| Error | ||
| end; | ||
| {error, _Reason} = Error -> | ||
| Error | ||
| end. | ||
|
|
||
| %% @private | ||
| %% Block the calling process until Socket is writable (or closed). | ||
| wait_write(Socket) -> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Combined with send_all, socket:send/2 can now block forever. OTP offers send/3,4 with Timeout/nowait returning {error, {timeout, Rest}} or {select, ...}. |
||
| Ref = erlang:make_ref(), | ||
| case ?MODULE:nif_select_write(Socket, Ref) of | ||
| ok -> | ||
| receive | ||
| {'$socket', Socket, select, Ref} -> | ||
| ok; | ||
| {'$socket', Socket, abort, {Ref, Reason}} -> | ||
| {error, Reason} | ||
| end; | ||
| {error, _Reason} = Error -> | ||
| Error | ||
| end. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
socket:send/2 blocks with no timeout and runs inside the gen_server, which other processes reach with gen_server:call(..., infinity). Previously this returned {ok, Rest} at once; now gen_tcp:recv/2,3, close/1 and controlling_process/2 from any other process hang for as long as the peer does not read. Either perform the send in the caller's process (the state's socket can be returned to it), or do a nowait-style send and keep a pending-write queue in the state.