diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c82fb122c..f509354519 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added support for configuring pins and width for sdmmc on ESP32 - Added support for map comprehensions - Added USB CDC port drivers for ESP32, RP2, and STM32 platforms +- Implemented `enif_select_write` (previously declared but not implemented), and reworked + `enif_select`/`enif_select_read`/`enif_select_write` so a resource can hold independent + pending read and write selects (each with its own ref/message/pid) on the same event at + the same time, instead of one direction silently overwriting the other's state ### Changed - Updated network type db() to dbm() to reflect the actual representation of the type @@ -66,6 +70,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed several underallocation issues that could trigger data corruption on `binary:replace`, `zlib:compress` and bsd socket recv code. - Fixed a bug where `catch` would raise on regular atom results - Fixed ESP32 socket driver holding the global socket-list lock across blocking TCP connects, leaking the port on connect failure, losing concurrent `accept` waiters, leaking `netbuf` on receive error paths, and a recycled-`netconn` race between socket close and the event handler +- `socket:send/2,3` now correctly distinguishes transient send backpressure (lwIP `ERR_MEM` / BSD + `EAGAIN`|`EWOULDBLOCK`) from a closed connection at the NIF level, and waits for the socket to + become writable again (using a new `nif_select_write/2`) and retries internally, so callers get + `ok | {error, Reason}` and no longer need to implement their own retry/backoff for partial sends + or backpressure; `ssl:send/2`, `ssl:recv/2` and the TLS handshake/close-notify loops likewise wait + for write-readiness instead of busy-looping on `want_write` +- Fixed Erlang distribution over sockets (`socket_dist_controller`) silently ignoring + `socket:send/2` errors on `tick` and outgoing distribution data; the connection is now + terminated instead of continuing to run against a broken socket ## [0.7.0-alpha.1] - 2026-04-06 diff --git a/examples/erlang/rp2/picow_tcp_server.erl b/examples/erlang/rp2/picow_tcp_server.erl index 053bf170c2..79ff6923a8 100644 --- a/examples/erlang/rp2/picow_tcp_server.erl +++ b/examples/erlang/rp2/picow_tcp_server.erl @@ -78,8 +78,6 @@ echo(ConnectedSocket) -> case socket:send(ConnectedSocket, Data) of ok -> io:format("All data was sent~n"); - {ok, Rest} -> - io:format("Some data was sent. Remaining: ~p~n", [Rest]); {error, Reason} -> io:format("An error occurred sending data: ~p~n", [Reason]) end, diff --git a/examples/erlang/tcp_socket_server.erl b/examples/erlang/tcp_socket_server.erl index 96fdb53a1e..029e9b6f9f 100644 --- a/examples/erlang/tcp_socket_server.erl +++ b/examples/erlang/tcp_socket_server.erl @@ -61,8 +61,6 @@ echo(ConnectedSocket) -> case socket:send(ConnectedSocket, Data) of ok -> io:format("All data was sent~n"); - {ok, Rest} -> - io:format("Some data was sent. Remaining: ~p~n", [Rest]); {error, Reason} -> io:format("An error occurred sending data: ~p~n", [Reason]) end, diff --git a/examples/erlang/udp_socket_client.erl b/examples/erlang/udp_socket_client.erl index 3389d31b11..a4b6df97ad 100644 --- a/examples/erlang/udp_socket_client.erl +++ b/examples/erlang/udp_socket_client.erl @@ -37,10 +37,6 @@ loop(Socket) -> io:format("Send packet ~p to ~p.~n", [ Data, Dest ]); - {ok, Rest} -> - io:format("Send packet ~p to ~p. Remaining: ~p~n", [ - Data, Dest, Rest - ]); {error, Reason} -> io:format("An error occurred sending a packet: ~p~n", [Reason]) end, diff --git a/libs/avm_network/src/epmd.erl b/libs/avm_network/src/epmd.erl index b012fe1eb7..fc9cc839cb 100644 --- a/libs/avm_network/src/epmd.erl +++ b/libs/avm_network/src/epmd.erl @@ -157,7 +157,7 @@ process_req( ) -> case socket:recv(Socket, 1, nowait) of {select, {select_info, recv, Ref}} -> - socket:send(Socket, <>), + ok = socket:send(Socket, <>), State#state{ recv_selects = [{Ref, client} | RecvSelects], clients = [ diff --git a/libs/estdlib/src/socket.erl b/libs/estdlib/src/socket.erl index 810cf563c2..9fe0ba17ab 100644 --- a/libs/estdlib/src/socket.erl +++ b/libs/estdlib/src/socket.erl @@ -46,6 +46,7 @@ %% internal nifs -export([ nif_select_read/2, + nif_select_write/2, nif_accept/1, nif_recv/2, nif_recvfrom/2, @@ -571,7 +572,7 @@ 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. %% @doc Send data on the specified socket. %% %% Note that this function will block until data is sent @@ -579,23 +580,29 @@ recvfrom0_nowait(Socket, Length, Ref) -> %% 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()}. 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 @@ -603,17 +610,20 @@ send(Socket, Data) -> %% 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); 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 +718,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). @@ -731,3 +745,62 @@ nif_send(_Socket, _Data) -> %% @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. +send_all(_Socket, <<>>) -> + ok; +send_all(Socket, Data) -> + case ?MODULE:nif_send(Socket, Data) of + ok -> + ok; + {ok, Rest} -> + send_all(Socket, Rest); + {error, eagain} -> + case wait_write(Socket) of + ok -> + send_all(Socket, Data); + {error, _Reason} = Error -> + Error + end; + {error, _Reason} = Error -> + Error + end. + +%% @private +sendto_all(_Socket, <<>>, _Dest) -> + ok; +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) -> + 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. diff --git a/libs/estdlib/src/socket_dist_controller.erl b/libs/estdlib/src/socket_dist_controller.erl index d97f7eb59b..3e66782025 100644 --- a/libs/estdlib/src/socket_dist_controller.erl +++ b/libs/estdlib/src/socket_dist_controller.erl @@ -142,8 +142,12 @@ handle_call(getstat, _From, #state{received = Received, sent = Sent} = State) -> {reply, {ok, Received, Sent, 0}, State}. handle_cast(tick, #state{socket = Socket, sent = Sent} = State) -> - socket:send(Socket, <<0:32>>), - {noreply, State#state{sent = Sent + 1}}; + case socket:send(Socket, <<0:32>>) of + ok -> + {noreply, State#state{sent = Sent + 1}}; + {error, Reason} -> + {stop, {send_error, Reason}, State} + end; handle_cast({handshake_complete, DHandle}, State0) -> ok = erlang:dist_ctrl_get_data_notification(DHandle), % We now need to get messages when data is coming. @@ -154,8 +158,10 @@ handle_cast({handshake_complete, DHandle}, State0) -> end. handle_info(dist_data, State0) -> - State1 = send_data_loop(State0), - {noreply, State1}; + case send_data_loop(State0) of + {ok, State1} -> {noreply, State1}; + {stop, Reason, State1} -> {stop, Reason, State1} + end; handle_info( {'$socket', Socket, select, SelectHandle}, #state{socket = Socket, select_handle = SelectHandle} = State0 @@ -209,10 +215,14 @@ send_data_loop(#state{dhandle = DHandle, socket = Socket, sent = Sent} = State) case erlang:dist_ctrl_get_data(DHandle) of none -> ok = erlang:dist_ctrl_get_data_notification(DHandle), - State; + {ok, State}; Data -> DataBin = ?PRE_PROCESS(Data), DataSize = byte_size(DataBin), - socket:send(Socket, <>), - send_data_loop(State#state{sent = Sent + 1}) + case socket:send(Socket, <>) of + ok -> + send_data_loop(State#state{sent = Sent + 1}); + {error, Reason} -> + {stop, {send_error, Reason}, State} + end end. diff --git a/libs/estdlib/src/ssl.erl b/libs/estdlib/src/ssl.erl index 46385b0662..90ffc39506 100644 --- a/libs/estdlib/src/ssl.erl +++ b/libs/estdlib/src/ssl.erl @@ -200,8 +200,13 @@ handshake_loop(SSLContext, Socket) -> Error end; want_write -> - % We're currrently missing non-blocking writes - handshake_loop(SSLContext, Socket); + case wait_write(Socket) of + ok -> + handshake_loop(SSLContext, Socket); + {error, _Reason} = Error -> + socket:close(Socket), + Error + end; {error, _Reason} = Error -> socket:close(Socket), Error @@ -253,8 +258,13 @@ close_notify_loop(SSLContext, Socket) -> Error end; want_write -> - % We're currrently missing non-blocking writes - close_notify_loop(SSLContext, Socket); + case wait_write(Socket) of + ok -> + close_notify_loop(SSLContext, Socket); + {error, _Reason} = Error -> + socket:close(Socket), + Error + end; {error, _Reason} = Error -> socket:close(Socket), Error @@ -283,8 +293,12 @@ send({SSLContext, Socket} = SSLSocket, Binary) -> Error end; want_write -> - % We're currrently missing non-blocking writes - send(SSLSocket, Binary); + case wait_write(Socket) of + ok -> + send(SSLSocket, Binary); + {error, _Reason} = Error -> + Error + end; {error, _Reason} = Error -> Error end. @@ -318,8 +332,27 @@ recv0({SSLContext, Socket} = SSLSocket, Length, Remaining, Acc) -> Error end; want_write -> - % We're currrently missing non-blocking writes - recv0(SSLSocket, Length, Remaining, Acc); + case wait_write(Socket) of + ok -> + recv0(SSLSocket, Length, Remaining, Acc); + {error, _Reason} = Error -> + Error + end; + {error, _Reason} = Error -> + Error + end. + +%% @private +wait_write(Socket) -> + Ref = erlang:make_ref(), + case socket: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. diff --git a/src/libAtomVM/otp_socket.c b/src/libAtomVM/otp_socket.c index 4195a96241..2672fc722c 100644 --- a/src/libAtomVM/otp_socket.c +++ b/src/libAtomVM/otp_socket.c @@ -113,6 +113,7 @@ enum SocketState SocketStateSelectingRead = 1 << 3, SocketStateConnected = 1 << 4, SocketStateListening = 1 << 5, + SocketStateSelectingWrite = 1 << 6, // Actual states SocketStateUDPIdle = SocketStateUDP, @@ -120,6 +121,8 @@ enum SocketState SocketStateTCPNew = SocketStateTCP, SocketStateTCPConnected = SocketStateTCP | SocketStateConnected, SocketStateTCPSelectingRead = SocketStateTCPConnected | SocketStateSelectingRead, + SocketStateTCPSelectingWrite = SocketStateTCPConnected | SocketStateSelectingWrite, + SocketStateTCPSelectingReadWrite = SocketStateTCPConnected | SocketStateSelectingRead | SocketStateSelectingWrite, SocketStateTCPListening = SocketStateTCP | SocketStateListening, SocketStateTCPSelectingAccept = SocketStateTCPListening | SocketStateSelectingRead, }; @@ -146,8 +149,11 @@ struct UDPReceivedItem }; static err_t tcp_recv_cb(void *arg, struct tcp_pcb *tpcb, struct pbuf *p, err_t err); +static err_t tcp_sent_cb(void *arg, struct tcp_pcb *tpcb, u16_t len); +static err_t tcp_poll_cb(void *arg, struct tcp_pcb *tpcb); static void udp_recv_cb(void *arg, struct udp_pcb *pcb, struct pbuf *p, const ip_addr_t *addr, u16_t port); static void tcp_err_cb(void *arg, err_t err); +static bool lwip_tcp_can_send(struct SocketResource *rsrc_obj); #endif @@ -156,7 +162,15 @@ struct SocketResource { int fd; uint64_t socket_ref_ticks; - uint64_t select_ref_ticks; + uint64_t read_select_ref_ticks; + uint64_t write_select_ref_ticks; + // Track which direction(s) are actually armed, since a ref of 0 is + // ambiguous between "not selecting" and "selecting with an undefined ref". + // This allows a socket to have an independent pending read select (e.g. a + // permanent recv select) and a pending write select (e.g. transient send + // backpressure) at the same time, from the same owning process. + bool read_selecting; + bool write_selecting; int32_t selecting_process_id; ErlNifMonitor selecting_process_monitor; size_t buf_size; @@ -174,7 +188,8 @@ struct SocketResource struct udp_pcb *udp_pcb; }; uint64_t socket_ref_ticks; - uint64_t select_ref_ticks; + uint64_t read_select_ref_ticks; + uint64_t write_select_ref_ticks; int32_t selecting_process_id; // trapped or selecting ErlNifMonitor selecting_process_monitor; bool linger_on; @@ -247,7 +262,7 @@ static const AtomStringIntPair otp_socket_setopt_level_table[] = { static ErlNifResourceType *socket_resource_type; #define SOCKET_MAKE_SELECT_NOTIFICATION_SIZE (TUPLE_SIZE(4) + REF_SIZE + TUPLE_SIZE(2) + REF_SIZE + TERM_BOXED_REFERENCE_RESOURCE_SIZE) -static term socket_make_select_notification(struct SocketResource *rsrc_obj, Heap *heap); +static term socket_make_select_notification(struct SocketResource *rsrc_obj, Heap *heap, bool is_write); // // resource operations @@ -377,7 +392,7 @@ static const ErlNifResourceTypeInit SocketResourceTypeInit = { }; // Make a notification message, using SOCKET_MAKE_SELECT_NOTIFICATION_SIZE on heap -static term socket_make_select_notification(struct SocketResource *rsrc_obj, Heap *heap) +static term socket_make_select_notification(struct SocketResource *rsrc_obj, Heap *heap, bool is_write) { term notification = term_alloc_tuple(4, heap); term_put_tuple_element(notification, 0, DOLLAR_SOCKET_ATOM); @@ -392,11 +407,12 @@ static term socket_make_select_notification(struct SocketResource *rsrc_obj, Hea term_put_tuple_element(socket_tuple, 1, socket_ref); term_put_tuple_element(notification, 1, socket_tuple); term_put_tuple_element(notification, 2, SELECT_ATOM); + uint64_t select_ref_ticks = is_write ? rsrc_obj->write_select_ref_ticks : rsrc_obj->read_select_ref_ticks; term select_ref; - if (rsrc_obj->select_ref_ticks == 0) { + if (select_ref_ticks == 0) { select_ref = UNDEFINED_ATOM; } else { - select_ref = term_from_ref_ticks(rsrc_obj->select_ref_ticks, heap); + select_ref = term_from_ref_ticks(select_ref_ticks, heap); } term_put_tuple_element(notification, 3, select_ref); return notification; @@ -404,20 +420,20 @@ static term socket_make_select_notification(struct SocketResource *rsrc_obj, Hea // select emulation for lwIP that doesn't have select. #if OTP_SOCKET_LWIP -static void select_event_send_notification_from_nif(struct SocketResource *rsrc_obj, Context *locked_ctx) +static void select_event_send_notification_from_nif(struct SocketResource *rsrc_obj, Context *locked_ctx, bool is_write) { BEGIN_WITH_STACK_HEAP(SOCKET_MAKE_SELECT_NOTIFICATION_SIZE, heap) - term notification = socket_make_select_notification(rsrc_obj, &heap); + term notification = socket_make_select_notification(rsrc_obj, &heap, is_write); mailbox_send(locked_ctx, notification); END_WITH_STACK_HEAP(heap, locked_ctx->global) } -static void select_event_send_notification_from_handler(struct SocketResource *rsrc_obj, int32_t process_id) +static void select_event_send_notification_from_handler(struct SocketResource *rsrc_obj, int32_t process_id, bool is_write) { struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj); GlobalContext *global = rsrc_refc->resource_type->global; BEGIN_WITH_STACK_HEAP(SOCKET_MAKE_SELECT_NOTIFICATION_SIZE, heap) - term notification = socket_make_select_notification(rsrc_obj, &heap); + term notification = socket_make_select_notification(rsrc_obj, &heap, is_write); globalcontext_send_message(global, process_id, notification); END_WITH_STACK_HEAP(heap, global) } @@ -588,6 +604,10 @@ static term nif_socket_open(Context *ctx, int argc, term argv[]) } else { TRACE("nif_socket_open: Created socket fd=%i\n", rsrc_obj->fd); rsrc_obj->selecting_process_id = INVALID_PROCESS_ID; + rsrc_obj->read_select_ref_ticks = 0; + rsrc_obj->write_select_ref_ticks = 0; + rsrc_obj->read_selecting = false; + rsrc_obj->write_selecting = false; if (type != SOCK_STREAM) { // TCP sockets are made non-blocking after connect, for now. @@ -619,6 +639,8 @@ static term nif_socket_open(Context *ctx, int argc, term argv[]) RAISE_ERROR(OUT_OF_MEMORY_ATOM); } else { rsrc_obj->selecting_process_id = INVALID_PROCESS_ID; + rsrc_obj->read_select_ref_ticks = 0; + rsrc_obj->write_select_ref_ticks = 0; rsrc_obj->linger_on = false; rsrc_obj->linger_sec = 0; rsrc_obj->pos = 0; @@ -628,6 +650,8 @@ static term nif_socket_open(Context *ctx, int argc, term argv[]) tcp_arg(rsrc_obj->tcp_pcb, rsrc_obj); tcp_err(rsrc_obj->tcp_pcb, tcp_err_cb); tcp_recv(rsrc_obj->tcp_pcb, tcp_recv_cb); + tcp_sent(rsrc_obj->tcp_pcb, tcp_sent_cb); + tcp_poll(rsrc_obj->tcp_pcb, tcp_poll_cb, 1); LWIP_END(); } else { LWIP_BEGIN(); @@ -692,7 +716,7 @@ bool term_is_otp_socket(term socket_term) // close // -static int send_closed_notification(Context *ctx, term socket_term, int32_t selecting_process_id, struct SocketResource *rsrc_obj) +static int send_closed_notification(Context *ctx, term socket_term, int32_t selecting_process_id, struct SocketResource *rsrc_obj, bool is_write) { // send a {'$socket', Socket, abort, {Ref | undefined, closed}} message to the pid if (UNLIKELY(memory_ensure_free_with_roots(ctx, TUPLE_SIZE(4) + TUPLE_SIZE(2) + REF_SIZE, 1, &socket_term, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { @@ -706,7 +730,8 @@ static int send_closed_notification(Context *ctx, term socket_term, int32_t sele term_put_tuple_element(socket_tuple, 2, ABORT_ATOM); term error_tuple = term_alloc_tuple(2, &ctx->heap); - term ref = (rsrc_obj->select_ref_ticks == 0) ? UNDEFINED_ATOM : term_from_ref_ticks(rsrc_obj->select_ref_ticks, &ctx->heap); + uint64_t select_ref_ticks = is_write ? rsrc_obj->write_select_ref_ticks : rsrc_obj->read_select_ref_ticks; + term ref = (select_ref_ticks == 0) ? UNDEFINED_ATOM : term_from_ref_ticks(select_ref_ticks, &ctx->heap); term_put_tuple_element(error_tuple, 0, ref); term_put_tuple_element(error_tuple, 1, CLOSED_ATOM); term_put_tuple_element(socket_tuple, 3, error_tuple); @@ -770,11 +795,24 @@ static term nif_socket_close(Context *ctx, int argc, term argv[]) // if (selecting_process_id != ctx->process_id) { // send a {'$socket', Socket, abort, {Ref | undefined, closed}} message to the pid - if (UNLIKELY(send_closed_notification(ctx, argv[0], selecting_process_id, rsrc_obj) < 0)) { - SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); - RAISE_ERROR(OUT_OF_MEMORY_ATOM); + // enif_select with ERL_NIF_SELECT_STOP above stopped both read + // and write (if either was armed), so notify whichever + // direction(s) were actually selecting. + if (rsrc_obj->read_selecting) { + if (UNLIKELY(send_closed_notification(ctx, argv[0], selecting_process_id, rsrc_obj, false) < 0)) { + SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); + RAISE_ERROR(OUT_OF_MEMORY_ATOM); + } + } + if (rsrc_obj->write_selecting) { + if (UNLIKELY(send_closed_notification(ctx, argv[0], selecting_process_id, rsrc_obj, true) < 0)) { + SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); + RAISE_ERROR(OUT_OF_MEMORY_ATOM); + } } } + rsrc_obj->read_selecting = false; + rsrc_obj->write_selecting = false; // Now, ref_count >= 1 only. } @@ -790,13 +828,21 @@ static term nif_socket_close(Context *ctx, int argc, term argv[]) } #elif OTP_SOCKET_LWIP // If the socket is being selected by another process, send a closed notification. - if (rsrc_obj->socket_state & SocketStateSelectingRead + if ((rsrc_obj->socket_state & (SocketStateSelectingRead | SocketStateSelectingWrite)) && rsrc_obj->selecting_process_id != INVALID_PROCESS_ID && rsrc_obj->selecting_process_id != ctx->process_id) { // send a {'$socket', Socket, abort, {Ref | undefined, closed}} message to the pid - if (UNLIKELY(send_closed_notification(ctx, argv[0], rsrc_obj->selecting_process_id, rsrc_obj) < 0)) { - SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); - RAISE_ERROR(OUT_OF_MEMORY_ATOM); + if (rsrc_obj->socket_state & SocketStateSelectingRead) { + if (UNLIKELY(send_closed_notification(ctx, argv[0], rsrc_obj->selecting_process_id, rsrc_obj, false) < 0)) { + SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); + RAISE_ERROR(OUT_OF_MEMORY_ATOM); + } + } + if (rsrc_obj->socket_state & SocketStateSelectingWrite) { + if (UNLIKELY(send_closed_notification(ctx, argv[0], rsrc_obj->selecting_process_id, rsrc_obj, true) < 0)) { + SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); + RAISE_ERROR(OUT_OF_MEMORY_ATOM); + } } } if (rsrc_obj->socket_state == SocketStateClosed) { @@ -849,6 +895,8 @@ static struct SocketResource *make_accepted_socket_resource(struct tcp_pcb *newp conn_rsrc_obj->socket_state = SocketStateTCPConnected; conn_rsrc_obj->tcp_pcb = newpcb; conn_rsrc_obj->selecting_process_id = INVALID_PROCESS_ID; + conn_rsrc_obj->read_select_ref_ticks = 0; + conn_rsrc_obj->write_select_ref_ticks = 0; conn_rsrc_obj->pos = 0; conn_rsrc_obj->linger_on = false; conn_rsrc_obj->linger_sec = 0; @@ -864,6 +912,8 @@ static struct SocketResource *make_accepted_socket_resource(struct tcp_pcb *newp tcp_arg(newpcb, conn_rsrc_obj); tcp_recv(newpcb, tcp_recv_cb); + tcp_sent(newpcb, tcp_sent_cb); + tcp_poll(newpcb, tcp_poll_cb, 1); return conn_rsrc_obj; } @@ -879,7 +929,7 @@ static void tcp_accept_handler(struct LWIPEvent *event) // Clear flag to avoid sending a message again. rsrc_obj->socket_state &= ~SocketStateSelectingRead; if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) { - select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id); + select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id, false); } // otherwise, selecting process died but we can just wait for monitor to handle it } } @@ -942,7 +992,7 @@ static void tcp_recv_handler(struct LWIPEvent *event) // Clear flag to avoid sending a message again. rsrc_obj->socket_state &= ~SocketStateSelectingRead; if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) { - select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id); + select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id, false); } // otherwise, selecting process died but we can just wait for monitor to handle it } } @@ -963,6 +1013,63 @@ static err_t tcp_recv_cb(void *arg, struct tcp_pcb *tpcb, struct pbuf *p, err_t return ERR_OK; } +// Returns true if the lwIP TCP send buffer has room for more data, i.e. a +// subsequent tcp_write/tcp_output is likely to succeed rather than return +// ERR_MEM. +static bool lwip_tcp_can_send(struct SocketResource *rsrc_obj) +{ + return tcp_sndbuf(rsrc_obj->tcp_pcb) > 0 && tcp_sndqueuelen(rsrc_obj->tcp_pcb) < TCP_SND_QUEUELEN; +} + +static void tcp_sent_handler(struct LWIPEvent *event) +{ + struct SocketResource *rsrc_obj = event->tcp_sent.rsrc_obj; + + // Send notification if we are selecting for write and the send buffer + // has room again. + if ((rsrc_obj->socket_state & SocketStateSelectingWrite) && lwip_tcp_can_send(rsrc_obj)) { + // Clear flag to avoid sending a message again. + rsrc_obj->socket_state &= ~SocketStateSelectingWrite; + if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) { + select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id, true); + } // otherwise, selecting process died but we can just wait for monitor to handle it + } +} + +// Called by lwIP when previously written data has been acknowledged by the +// peer, freeing up room in the send buffer. +static err_t tcp_sent_cb(void *arg, struct tcp_pcb *tpcb, u16_t len) +{ + UNUSED(tpcb); + UNUSED(len); + + struct SocketResource *rsrc_obj = (struct SocketResource *) arg; + if (LIKELY(rsrc_obj)) { + struct LWIPEvent event; + event.handler = tcp_sent_handler; + event.tcp_sent.rsrc_obj = rsrc_obj; + otp_socket_lwip_enqueue(&event); + } + return ERR_OK; +} + +// Periodic poll callback, used as a fallback wakeup for write-readiness: some +// lwIP send failure paths (e.g. hitting TCP_SND_QUEUELEN without any bytes +// being freshly acknowledged) would otherwise never trigger tcp_sent_cb. +static err_t tcp_poll_cb(void *arg, struct tcp_pcb *tpcb) +{ + UNUSED(tpcb); + + struct SocketResource *rsrc_obj = (struct SocketResource *) arg; + if (LIKELY(rsrc_obj) && (rsrc_obj->socket_state & SocketStateSelectingWrite)) { + struct LWIPEvent event; + event.handler = tcp_sent_handler; + event.tcp_sent.rsrc_obj = rsrc_obj; + otp_socket_lwip_enqueue(&event); + } + return ERR_OK; +} + static void udp_recv_handler(struct LWIPEvent *event) { struct SocketResource *rsrc_obj = event->udp_recv.rsrc_obj; @@ -977,7 +1084,7 @@ static void udp_recv_handler(struct LWIPEvent *event) // Clear flag to avoid sending a message again. rsrc_obj->socket_state &= ~SocketStateSelectingRead; if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) { - select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id); + select_event_send_notification_from_handler(rsrc_obj, rsrc_obj->selecting_process_id, false); } // otherwise, selecting process died } } @@ -1021,9 +1128,9 @@ int tcpip_try_callback(tcpip_callback_fn function, void *ctx) #endif -static term nif_socket_select_read(Context *ctx, int argc, term argv[]) +static term nif_socket_select(Context *ctx, int argc, term argv[], bool is_write) { - TRACE("nif_socket_select_read\n"); + TRACE("nif_socket_select is_write=%i\n", (int) is_write); UNUSED(argc); @@ -1062,22 +1169,30 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[]) rsrc_obj->selecting_process_id = ctx->process_id; } - rsrc_obj->select_ref_ticks = (select_ref_term == UNDEFINED_ATOM) ? 0 : term_to_ref_ticks(select_ref_term); + uint64_t select_ref_ticks = (select_ref_term == UNDEFINED_ATOM) ? 0 : term_to_ref_ticks(select_ref_term); + if (is_write) { + rsrc_obj->write_select_ref_ticks = select_ref_ticks; + } else { + rsrc_obj->read_select_ref_ticks = select_ref_ticks; + } #if OTP_SOCKET_BSD TRACE("rsrc_obj->fd=%i\n", (int) rsrc_obj->fd); // The socket may be closed here. if (rsrc_obj->fd == CLOSED_FD) { - send_closed_notification(ctx, argv[0], ctx->process_id, rsrc_obj); + send_closed_notification(ctx, argv[0], ctx->process_id, rsrc_obj, is_write); } else { if (UNLIKELY(memory_ensure_free_with_roots(ctx, SOCKET_MAKE_SELECT_NOTIFICATION_SIZE, 2, argv, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { AVM_LOGW(TAG, "Failed to allocate memory: %s:%i.", __FILE__, __LINE__); SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); RAISE_ERROR(OUT_OF_MEMORY_ATOM); } - term notification = socket_make_select_notification(rsrc_obj, &ctx->heap); - if (UNLIKELY(enif_select_read(erl_nif_env_from_context(ctx), rsrc_obj->fd, rsrc_obj, &ctx->process_id, notification, NULL) < 0)) { + term notification = socket_make_select_notification(rsrc_obj, &ctx->heap, is_write); + int select_result = is_write + ? enif_select_write(erl_nif_env_from_context(ctx), rsrc_obj->fd, rsrc_obj, &ctx->process_id, notification, NULL) + : enif_select_read(erl_nif_env_from_context(ctx), rsrc_obj->fd, rsrc_obj, &ctx->process_id, notification, NULL); + if (UNLIKELY(select_result < 0)) { if (LIKELY(enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) { refc_binary_decrement_refcount(rsrc_refc, ctx->global); } @@ -1085,47 +1200,29 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[]) SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); RAISE_ERROR(BADARG_ATOM); } + if (is_write) { + rsrc_obj->write_selecting = true; + } else { + rsrc_obj->read_selecting = true; + } } #elif OTP_SOCKET_LWIP LWIP_BEGIN(); - switch (rsrc_obj->socket_state) { - case SocketStateTCPListening: { - if (!list_is_empty(&rsrc_obj->received_list)) { - // Send (or resend) notification - select_event_send_notification_from_nif(rsrc_obj, ctx); - } else { - // Set flag to send it when a packet will arrive. - rsrc_obj->socket_state = SocketStateTCPSelectingAccept; - } - } break; - case SocketStateTCPSelectingAccept: - // noop - break; - case SocketStateTCPConnected: { - if (!list_is_empty(&rsrc_obj->received_list)) { + if (is_write) { + if (rsrc_obj->socket_state & SocketStateTCPConnected) { + if (lwip_tcp_can_send(rsrc_obj)) { // Send (or resend) notification - select_event_send_notification_from_nif(rsrc_obj, ctx); + select_event_send_notification_from_nif(rsrc_obj, ctx, true); } else { - // Set flag to send it when a packet will arrive. - rsrc_obj->socket_state = SocketStateTCPSelectingRead; + // Set flag to send it when the send buffer has room again. + rsrc_obj->socket_state |= SocketStateSelectingWrite; } - } break; - case SocketStateTCPSelectingRead: - // noop - break; - case SocketStateUDPIdle: { - if (!list_is_empty(&rsrc_obj->received_list)) { - // Send (or resend) notification - select_event_send_notification_from_nif(rsrc_obj, ctx); - } else { - rsrc_obj->socket_state = SocketStateUDPSelectingRead; - } - } break; - case SocketStateUDPSelectingRead: - // noop - break; - default: + } else if (rsrc_obj->socket_state & SocketStateUDP) { + // UDP datagrams are never buffered by lwIP in a way that blocks + // the caller, so writability is immediate. + select_event_send_notification_from_nif(rsrc_obj, ctx, true); + } else { if (LIKELY(enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) { refc_binary_decrement_refcount(rsrc_refc, ctx->global); } @@ -1133,6 +1230,56 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[]) LWIP_END(); SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); RAISE_ERROR(BADARG_ATOM); + } + } else { + switch (rsrc_obj->socket_state) { + case SocketStateTCPListening: { + if (!list_is_empty(&rsrc_obj->received_list)) { + // Send (or resend) notification + select_event_send_notification_from_nif(rsrc_obj, ctx, false); + } else { + // Set flag to send it when a packet will arrive. + rsrc_obj->socket_state = SocketStateTCPSelectingAccept; + } + } break; + case SocketStateTCPSelectingAccept: + // noop + break; + case SocketStateTCPConnected: + case SocketStateTCPSelectingWrite: { + if (!list_is_empty(&rsrc_obj->received_list)) { + // Send (or resend) notification + select_event_send_notification_from_nif(rsrc_obj, ctx, false); + } else { + // Set flag to send it when a packet will arrive, preserving + // a pending write select on the same socket. + rsrc_obj->socket_state |= SocketStateSelectingRead; + } + } break; + case SocketStateTCPSelectingRead: + case SocketStateTCPSelectingReadWrite: + // noop + break; + case SocketStateUDPIdle: { + if (!list_is_empty(&rsrc_obj->received_list)) { + // Send (or resend) notification + select_event_send_notification_from_nif(rsrc_obj, ctx, false); + } else { + rsrc_obj->socket_state = SocketStateUDPSelectingRead; + } + } break; + case SocketStateUDPSelectingRead: + // noop + break; + default: + if (LIKELY(enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) { + refc_binary_decrement_refcount(rsrc_refc, ctx->global); + } + rsrc_obj->selecting_process_id = INVALID_PROCESS_ID; + LWIP_END(); + SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); + RAISE_ERROR(BADARG_ATOM); + } } LWIP_END(); #endif @@ -1141,6 +1288,16 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[]) return OK_ATOM; } +static term nif_socket_select_read(Context *ctx, int argc, term argv[]) +{ + return nif_socket_select(ctx, argc, argv, false); +} + +static term nif_socket_select_write(Context *ctx, int argc, term argv[]) +{ + return nif_socket_select(ctx, argc, argv, true); +} + static term nif_socket_select_stop(Context *ctx, int argc, term argv[]) { TRACE("nif_socket_stop\n"); @@ -1161,16 +1318,23 @@ static term nif_socket_select_stop(Context *ctx, int argc, term argv[]) } rsrc_obj->selecting_process_id = INVALID_PROCESS_ID; } + rsrc_obj->read_select_ref_ticks = 0; + rsrc_obj->write_select_ref_ticks = 0; #if OTP_SOCKET_BSD if (UNLIKELY(enif_select(erl_nif_env_from_context(ctx), rsrc_obj->fd, ERL_NIF_SELECT_STOP, rsrc_obj, NULL, term_nil()) < 0)) { SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); RAISE_ERROR(BADARG_ATOM); } + rsrc_obj->read_selecting = false; + rsrc_obj->write_selecting = false; #elif OTP_SOCKET_LWIP LWIP_BEGIN(); if (rsrc_obj->socket_state & SocketStateSelectingRead) { rsrc_obj->socket_state &= ~SocketStateSelectingRead; } + if (rsrc_obj->socket_state & SocketStateSelectingWrite) { + rsrc_obj->socket_state &= ~SocketStateSelectingWrite; + } LWIP_END(); #endif SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock); @@ -2573,6 +2737,13 @@ static term nif_socket_send_internal(Context *ctx, int argc, term argv[], bool i // {ok, RestData} | {error, Reason} + // Transient send backpressure (lwIP ERR_MEM / BSD EAGAIN|EWOULDBLOCK) is + // reported as {error, eagain} so callers can retry rather than mistaking it + // for a closed connection. + if (sent_data == SocketWouldBlock) { + return make_error_tuple(posix_errno_to_term(EAGAIN, global), ctx); + } + size_t rest_len = len - sent_data; if (rest_len == 0) { return OK_ATOM; @@ -2588,12 +2759,10 @@ static term nif_socket_send_internal(Context *ctx, int argc, term argv[], bool i return port_create_tuple2(ctx, OK_ATOM, rest); } else if (sent_data == 0) { - if (UNLIKELY(memory_ensure_free_with_roots(ctx, TUPLE_SIZE(2), 1, &data, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { - AVM_LOGW(TAG, "Failed to allocate memory: %s:%i.", __FILE__, __LINE__); - RAISE_ERROR(OUT_OF_MEMORY_ATOM); - } - - return port_create_tuple2(ctx, OK_ATOM, data); + // do_socket_send only returns 0 for a closed connection (SocketClosed: + // lwIP ERR_CLSD, BSD EBADF|ECONNRESET); an empty payload already + // returned ok above via the rest_len == 0 check. Report it as such. + return make_error_tuple(CLOSED_ATOM, ctx); } else { TRACE("Unable to send data: res=%zi.\n", sent_data); return make_error_tuple(CLOSED_ATOM, ctx); @@ -2930,6 +3099,10 @@ static const struct Nif socket_select_read_nif = { .base.type = NIFFunctionType, .nif_ptr = nif_socket_select_read }; +static const struct Nif socket_select_write_nif = { + .base.type = NIFFunctionType, + .nif_ptr = nif_socket_select_write +}; static const struct Nif socket_accept_nif = { .base.type = NIFFunctionType, .nif_ptr = nif_socket_accept @@ -3004,6 +3177,10 @@ const struct Nif *otp_socket_nif_get_nif(const char *nifname) TRACE("Resolved platform nif %s ...\n", nifname); return &socket_select_read_nif; } + if (strcmp("nif_select_write/2", rest) == 0) { + TRACE("Resolved platform nif %s ...\n", nifname); + return &socket_select_write_nif; + } if (strcmp("nif_accept/1", rest) == 0) { TRACE("Resolved platform nif %s ...\n", nifname); return &socket_accept_nif; diff --git a/src/libAtomVM/otp_socket.h b/src/libAtomVM/otp_socket.h index cfd9626531..ca8cd43f02 100644 --- a/src/libAtomVM/otp_socket.h +++ b/src/libAtomVM/otp_socket.h @@ -111,6 +111,10 @@ struct LWIPEvent err_t err; } tcp_recv; struct + { + struct SocketResource *rsrc_obj; + } tcp_sent; + struct { struct SocketResource *rsrc_obj; struct pbuf *buf; diff --git a/src/libAtomVM/resources.c b/src/libAtomVM/resources.c index 8ebabb5166..26d24abada 100644 --- a/src/libAtomVM/resources.c +++ b/src/libAtomVM/resources.c @@ -169,7 +169,8 @@ static int enif_select_common(ErlNifEnv *env, ErlNifEvent event, enum ErlNifSele resource->resource_type->stop(env, obj, event, true); } refc_binary_decrement_refcount(resource, global); - enif_select_event_message_dispose(select_event->message, global, false); + enif_select_event_message_dispose(select_event->read_message, global, false); + enif_select_event_message_dispose(select_event->write_message, global, false); free((void *) select_event); return ERL_NIF_SELECT_STOP_CALLED; } @@ -197,34 +198,45 @@ static int enif_select_common(ErlNifEnv *env, ErlNifEvent event, enum ErlNifSele } select_event->event = event; select_event->resource = resource; - select_event->message = NULL; - select_event->ref_ticks = 0; + select_event->read = false; + select_event->write = false; + select_event->close = false; + select_event->read_message = NULL; + select_event->write_message = NULL; + select_event->read_ref_ticks = 0; + select_event->write_ref_ticks = 0; + select_event->read_local_pid = INVALID_PROCESS_ID; + select_event->write_local_pid = INVALID_PROCESS_ID; // Resource is used in select_event, so we increase refcount. refc_binary_increment_refcount(resource); list_init(&select_event->head); list_append(select_events, &select_event->head); } - // Second read or second write overwrite ref/message & pid. - enif_select_event_message_dispose(select_event->message, global, false); - select_event->message = message; - if (message) { - select_event->ref_ticks = 0; - } else { - if (ref == UNDEFINED_ATOM) { - select_event->ref_ticks = 0; - } else { - select_event->ref_ticks = term_to_ref_ticks(ref); - } - } - select_event->local_pid = *pid; - select_event->read = mode & ERL_NIF_SELECT_READ; - select_event->write = mode & ERL_NIF_SELECT_WRITE; - select_event->close = 0; + // Second read or second write overwrites only that direction, preserving + // the other direction when both read and write are selected on the same + // event (e.g. a socket with a pending recv select and a pending send + // select at the same time). + uint64_t ref_ticks = (!message && term_is_local_reference(ref)) ? term_to_ref_ticks(ref) : 0; + if (mode & ERL_NIF_SELECT_READ) { + enif_select_event_message_dispose(select_event->read_message, global, false); + select_event->read_message = message; + select_event->read_ref_ticks = message ? 0 : ref_ticks; + select_event->read_local_pid = *pid; + select_event->read = true; + } + if (mode & ERL_NIF_SELECT_WRITE) { + enif_select_event_message_dispose(select_event->write_message, global, false); + select_event->write_message = message; + select_event->write_ref_ticks = message ? 0 : ref_ticks; + select_event->write_local_pid = *pid; + select_event->write = true; + } + select_event->close = false; synclist_unlock(&global->select_events); - if (select_event->read) { + if (mode & ERL_NIF_SELECT_READ) { sys_register_select_event(global, event, false); } - if (select_event->write) { + if (mode & ERL_NIF_SELECT_WRITE) { sys_register_select_event(global, event, true); } return 0; @@ -248,7 +260,17 @@ int enif_select_read(ErlNifEnv *env, ErlNifEvent event, void *obj, const ErlNifP } Message *message = mailbox_message_create_normal_message_from_term(msg); enum ErlNifSelectFlags mode = ERL_NIF_SELECT_READ; - return enif_select_common(env, event, mode, obj, pid, term_nil(), message); + return enif_select_common(env, event, mode, obj, pid, UNDEFINED_ATOM, message); +} + +int enif_select_write(ErlNifEnv *env, ErlNifEvent event, void *obj, const ErlNifPid *pid, ERL_NIF_TERM msg, ErlNifEnv *msg_env) +{ + if (UNLIKELY(msg_env != NULL)) { + return ERL_NIF_SELECT_BADARG; + } + Message *message = mailbox_message_create_normal_message_from_term(msg); + enum ErlNifSelectFlags mode = ERL_NIF_SELECT_WRITE; + return enif_select_common(env, event, mode, obj, pid, UNDEFINED_ATOM, message); } term select_event_make_notification(void *rsrc_obj, uint64_t ref_ticks, bool is_write, Heap *heap) @@ -269,25 +291,29 @@ term select_event_make_notification(void *rsrc_obj, uint64_t ref_ticks, bool is_ static void select_event_send_notification(struct SelectEvent *select_event, bool is_write, GlobalContext *global) { - if (select_event->message) { + Message **message = is_write ? &select_event->write_message : &select_event->read_message; + int32_t local_pid = is_write ? select_event->write_local_pid : select_event->read_local_pid; + uint64_t ref_ticks = is_write ? select_event->write_ref_ticks : select_event->read_ref_ticks; + + if (*message) { enum SendMessageResult result; #ifdef AVM_SELECT_IN_TASK - result = globalcontext_post_message_from_task(global, select_event->local_pid, select_event->message); + result = globalcontext_post_message_from_task(global, local_pid, *message); #else - result = globalcontext_post_message(global, select_event->local_pid, select_event->message); + result = globalcontext_post_message(global, local_pid, *message); #endif if (result == SEND_MESSAGE_OK) { // Ownership was properly transfered. // Otherwise, it will be destroyed when we have a context (when enif_select is called with stop for example) - select_event->message = NULL; + *message = NULL; } } else { BEGIN_WITH_STACK_HEAP(SELECT_EVENT_NOTIFICATION_SIZE, heap) - term notification = select_event_make_notification(select_event->resource->data, select_event->ref_ticks, is_write, &heap); + term notification = select_event_make_notification(select_event->resource->data, ref_ticks, is_write, &heap); #ifdef AVM_SELECT_IN_TASK - globalcontext_send_message_from_task(global, select_event->local_pid, NormalMessage, notification); + globalcontext_send_message_from_task(global, local_pid, NormalMessage, notification); #else - globalcontext_send_message(global, select_event->local_pid, notification); + globalcontext_send_message(global, local_pid, notification); #endif END_WITH_STACK_HEAP(heap, global) } @@ -338,7 +364,8 @@ static inline void select_event_destroy(struct SelectEvent *select_event, Global #else refc_binary_decrement_refcount(select_event->resource, global); #endif - enif_select_event_message_dispose(select_event->message, global, true); + enif_select_event_message_dispose(select_event->read_message, global, true); + enif_select_event_message_dispose(select_event->write_message, global, true); free((void *) select_event); } diff --git a/src/libAtomVM/resources.h b/src/libAtomVM/resources.h index 5c95a5c4ed..ed30055cb4 100644 --- a/src/libAtomVM/resources.h +++ b/src/libAtomVM/resources.h @@ -92,9 +92,12 @@ struct SelectEvent bool read; bool write; bool close; - int32_t local_pid; - uint64_t ref_ticks; - Message *message; + int32_t read_local_pid; + int32_t write_local_pid; + uint64_t read_ref_ticks; + uint64_t write_ref_ticks; + Message *read_message; + Message *write_message; }; /** diff --git a/tests/libs/estdlib/test_tcp_socket.erl b/tests/libs/estdlib/test_tcp_socket.erl index 4ceed655b9..b083d6c3f1 100644 --- a/tests/libs/estdlib/test_tcp_socket.erl +++ b/tests/libs/estdlib/test_tcp_socket.erl @@ -33,7 +33,8 @@ test() -> ok = test_setopt_getopt(), case erlang:system_info(machine) of "ATOM" -> - ok = test_abandon_select(); + ok = test_abandon_select(), + ok = test_send_backpressure(); "BEAM" -> ok end, @@ -570,5 +571,144 @@ test_abandon_select() -> erlang:garbage_collect(), ok. +%% +%% test_send_backpressure +%% +%% Exercises the write-select mechanism used internally by socket:send/2 to +%% wait for transient send backpressure (a full TCP send buffer) to clear, +%% instead of leaking {error, eagain} or {ok, Rest} to the caller. +%% + +test_send_backpressure() -> + etest:flush_msg_queue(), + + {ok, ListenSocket} = socket:open(inet, stream, tcp), + ok = socket:setopt(ListenSocket, {socket, reuseaddr}, true), + ok = socket:setopt(ListenSocket, {socket, linger}, #{onoff => true, linger => 0}), + ok = socket:bind(ListenSocket, #{family => inet, addr => loopback, port => 0}), + ok = socket:listen(ListenSocket), + {ok, #{port := Port}} = socket:sockname(ListenSocket), + + Self = self(), + Acceptor = spawn_link(fun() -> backpressure_acceptor(Self, ListenSocket) end), + + {ok, ClientSocket} = socket:open(inet, stream, tcp), + ok = try_connect(ClientSocket, Port, 10), + + ok = + receive + {server_socket, _ServerSocket} -> ok + after 5000 -> + error({timeout, waiting_for_server_socket}) + end, + + %% Fill the client's send buffer (the server never reads) using the raw + %% nif_send/2 directly, bypassing socket:send/2's automatic retry, until + %% we either observe a real {error, eagain} or give up after a generous + %% number of attempts. Different platforms/kernels size their socket + %% buffers differently, so we tolerate never observing backpressure + %% rather than failing the test outright. + Chunk = binary:copy(<<0>>, 65536), + {TotalSent, EAgainObserved} = fill_send_buffer(ClientSocket, Chunk, 0, 256), + + ok = + case EAgainObserved andalso TotalSent > 0 of + true -> + %% socket:nif_select_write/2 should let us wait until the + %% socket becomes writable again. Depending on how the + %% platform/kernel sizes and accounts for socket buffers, a + %% small partial drain on the peer may not be enough to + %% cross the low-water mark for writability, so we have the + %% acceptor drain everything (as a real reader normally + %% would) to reliably free up space. + Ref = erlang:make_ref(), + ok = socket:nif_select_write(ClientSocket, Ref), + + Acceptor ! {drain_all, self()}, + ok = + receive + {'$socket', ClientSocket, select, Ref} -> + ok + after 30000 -> + error({timeout, waiting_for_select_write}) + end, + + %% socket:send/2 should now transparently retry (internally + %% waiting for write-readiness as needed) and complete + %% successfully instead of returning {error, eagain} or + %% {ok, Rest}. + ok = socket:send(ClientSocket, Chunk), + ok = socket:close(ClientSocket), + ok = + receive + {drained_all, N} when is_integer(N) -> ok + after 30000 -> error({timeout, waiting_for_drain_all}) + end, + ok; + false -> + %% We never managed to fill the send buffer; nothing more to + %% verify on this platform. + Acceptor ! {drain_all, self()}, + ok = socket:close(ClientSocket), + ok = + receive + {drained_all, N} when is_integer(N) -> ok + after 30000 -> error({timeout, waiting_for_drain_all}) + end, + ok + end, + + ok. + +%% @private +%% Accepts a single connection and gives control of when to start reading +%% on it to the test process, so the test can deliberately stall the +%% receiver in order to build up send backpressure on the client side. +backpressure_acceptor(Owner, ListenSocket) -> + {ok, ServerSocket} = socket:accept(ListenSocket), + Owner ! {server_socket, ServerSocket}, + backpressure_acceptor_loop(Owner, ServerSocket), + ok = socket:close(ListenSocket). + +backpressure_acceptor_loop(Owner, ServerSocket) -> + receive + {drain_all, Owner} -> + Total = recv_until_closed(ServerSocket, 0), + Owner ! {drained_all, Total}, + ok = socket:close(ServerSocket) + after 60000 -> + ok = socket:close(ServerSocket) + end. + +recv_until_closed(Socket, Acc) -> + case socket:recv(Socket, 0, 30000) of + {ok, Data} -> + recv_until_closed(Socket, Acc + byte_size(Data)); + {error, closed} -> + Acc; + {error, timeout} -> + Acc + end. + +%% @private +%% Repeatedly calls the raw nif_send/2 (bypassing socket:send/2's automatic +%% retry) with the same chunk of data, until either an {error, eagain} is +%% observed (returns {TotalSent, true}) or MaxAttempts is reached without +%% ever observing backpressure (returns {TotalSent, false}). +fill_send_buffer(_Socket, _Chunk, TotalSent, 0) -> + {TotalSent, false}; +fill_send_buffer(Socket, Chunk, TotalSent, AttemptsLeft) -> + case socket:nif_send(Socket, Chunk) of + ok -> + fill_send_buffer(Socket, Chunk, TotalSent + byte_size(Chunk), AttemptsLeft - 1); + {ok, Rest} -> + Sent = byte_size(Chunk) - byte_size(Rest), + fill_send_buffer(Socket, Chunk, TotalSent + Sent, AttemptsLeft - 1); + {error, eagain} -> + {TotalSent, true}; + {error, Reason} -> + error({unexpected_send_error, Reason}) + end. + id(X) -> X. diff --git a/tests/test-enif.c b/tests/test-enif.c index 9bc79121c2..d72c46cfd2 100644 --- a/tests/test-enif.c +++ b/tests/test-enif.c @@ -20,6 +20,7 @@ #include #include +#include #include "context.h" #include "defaultatoms.h" @@ -29,6 +30,7 @@ #include "external_term.h" #include "globalcontext.h" #include "memory.h" +#include "resources.h" #include "scheduler.h" #include "utils.h" @@ -704,6 +706,226 @@ void test_resource_release_in_down_handler_two_monitors(void) globalcontext_destroy(glb); } +// enif_select_read/enif_select_write should track independent messages, refs +// and target pids for the same event, so a resource can have a pending read +// select and a pending write select at the same time (e.g. a socket with a +// permanent recv select and a transient send-backpressure select). +void test_resource_select_read_write_independent_messages(void) +{ + GlobalContext *glb = globalcontext_new(); + Context *read_ctx = context_new(glb); + Context *write_ctx = context_new(glb); + ErlNifEnv *env = erl_nif_env_from_context(read_ctx); + + ErlNifResourceTypeInit init; + init.members = 1; + init.dtor = resource_dtor; + ErlNifResourceFlags flags; + cb_read_resource = 0; + dtor_call_count = 0; + + ErlNifResourceType *resource_type = enif_init_resource_type(env, "test_select_resource", &init, ERL_NIF_RT_CREATE, &flags); + assert(resource_type != NULL); + + void *ptr = enif_alloc_resource(resource_type, sizeof(uint32_t)); + uint32_t *resource = (uint32_t *) ptr; + *resource = 99; + + int pipefds[2]; + assert(pipe(pipefds) == 0); + ErlNifEvent event = (ErlNifEvent) pipefds[0]; + + ErlNifPid read_pid = read_ctx->process_id; + ErlNifPid write_pid = write_ctx->process_id; + + term read_msg = term_from_int(11); + term write_msg = term_from_int(22); + + int r = enif_select_read(env, event, ptr, &read_pid, read_msg, NULL); + assert(r == 0); + r = enif_select_write(env, event, ptr, &write_pid, write_msg, NULL); + assert(r == 0); + + // Firing both read and write readiness must deliver the read message to + // read_ctx and the write message to write_ctx, without either clobbering + // the other (this is the bug being tested: a shared message/ref/pid field + // would either lose one notification or send it to the wrong process). + bool notified = select_event_notify(event, true, true, glb); + assert(notified); + + assert(mailbox_process_outer_list(&read_ctx->mailbox) == NULL); + assert(mailbox_has_next(&read_ctx->mailbox)); + term read_received; + assert(mailbox_peek(read_ctx, &read_received)); + assert(read_received == read_msg); + mailbox_remove_message(&read_ctx->mailbox, &read_ctx->heap); + assert(!mailbox_has_next(&read_ctx->mailbox)); + + assert(mailbox_process_outer_list(&write_ctx->mailbox) == NULL); + assert(mailbox_has_next(&write_ctx->mailbox)); + term write_received; + assert(mailbox_peek(write_ctx, &write_received)); + assert(write_received == write_msg); + mailbox_remove_message(&write_ctx->mailbox, &write_ctx->heap); + assert(!mailbox_has_next(&write_ctx->mailbox)); + + // Once a direction fires, only that direction is consumed; the other + // stays armed until it is separately triggered or explicitly stopped. + // Re-arm both to test the ref-ticks (no custom message) path together. + r = enif_select_read(env, event, ptr, &read_pid, read_msg, NULL); + assert(r == 0); + r = enif_select_write(env, event, ptr, &write_pid, write_msg, NULL); + assert(r == 0); + + // Re-selecting read must not disturb the pending write selection (and + // vice-versa): overwrite only the read side with a new pid/message. + Context *other_read_ctx = context_new(glb); + ErlNifPid other_read_pid = other_read_ctx->process_id; + term other_read_msg = term_from_int(33); + r = enif_select_read(env, event, ptr, &other_read_pid, other_read_msg, NULL); + assert(r == 0); + + notified = select_event_notify(event, true, true, glb); + assert(notified); + + // Original read_ctx should get nothing new (its select was overwritten). + assert(mailbox_process_outer_list(&read_ctx->mailbox) == NULL); + assert(!mailbox_has_next(&read_ctx->mailbox)); + + assert(mailbox_process_outer_list(&other_read_ctx->mailbox) == NULL); + assert(mailbox_has_next(&other_read_ctx->mailbox)); + term other_read_received; + assert(mailbox_peek(other_read_ctx, &other_read_received)); + assert(other_read_received == other_read_msg); + + assert(mailbox_process_outer_list(&write_ctx->mailbox) == NULL); + assert(mailbox_has_next(&write_ctx->mailbox)); + term write_received_again; + assert(mailbox_peek(write_ctx, &write_received_again)); + assert(write_received_again == write_msg); + mailbox_remove_message(&write_ctx->mailbox, &write_ctx->heap); + + // Both directions have now fired and are no longer armed, so stopping the + // select should immediately release the extra refcount it was holding. + int stop_result = enif_select(env, event, ERL_NIF_SELECT_STOP, ptr, NULL, term_nil()); + assert(stop_result == ERL_NIF_SELECT_STOP_CALLED); + + int release_result = enif_release_resource(ptr); + assert(release_result); + + scheduler_terminate(read_ctx); + scheduler_terminate(write_ctx); + scheduler_terminate(other_read_ctx); + + close(pipefds[0]); + close(pipefds[1]); + + globalcontext_destroy(glb); +} + +// The generic enif_select/2 API (no custom message, just a reference) must +// also track read and write refs independently when both directions are +// selected on the same event, e.g. a single ref selecting both directions, +// followed by a read-only re-select that must not clear the write ref. +void test_resource_select_read_write_independent_refs(void) +{ + GlobalContext *glb = globalcontext_new(); + Context *ctx = context_new(glb); + ErlNifEnv *env = erl_nif_env_from_context(ctx); + + ErlNifResourceTypeInit init; + init.members = 1; + init.dtor = resource_dtor; + ErlNifResourceFlags flags; + cb_read_resource = 0; + + ErlNifResourceType *resource_type = enif_init_resource_type(env, "test_select_resource_refs", &init, ERL_NIF_RT_CREATE, &flags); + assert(resource_type != NULL); + + void *ptr = enif_alloc_resource(resource_type, sizeof(uint32_t)); + uint32_t *resource = (uint32_t *) ptr; + *resource = 7; + + int pipefds[2]; + assert(pipe(pipefds) == 0); + ErlNifEvent event = (ErlNifEvent) pipefds[0]; + + ErlNifPid pid = ctx->process_id; + + assert(memory_erl_nif_env_ensure_free(env, REF_SIZE) == MEMORY_GC_OK); + term write_ref = term_from_ref_ticks(globalcontext_get_ref_ticks(glb), &env->heap); + + // Select for write with an explicit ref and no custom message. + int r = enif_select(env, event, ERL_NIF_SELECT_WRITE, ptr, &pid, write_ref); + assert(r == 0); + + // Now select read only, with UNDEFINED_ATOM (no ref requested). This must + // not disturb the write ref registered above. + r = enif_select(env, event, ERL_NIF_SELECT_READ, ptr, &pid, UNDEFINED_ATOM); + assert(r == 0); + + bool notified = select_event_notify(event, true, true, glb); + assert(notified); + + // Two notifications should have been queued: {select, Resource, undefined, ready_input} + // and {select, Resource, WriteRef, ready_output}. + assert(mailbox_process_outer_list(&ctx->mailbox) == NULL); + assert(mailbox_has_next(&ctx->mailbox)); + + bool saw_read = false; + bool saw_write = false; + for (int i = 0; i < 2; i++) { + term msg; + assert(mailbox_peek(ctx, &msg)); + assert(term_is_tuple(msg)); + assert(term_get_tuple_arity(msg) == 4); + assert(term_get_tuple_element(msg, 0) == SELECT_ATOM); + term ref_or_undefined = term_get_tuple_element(msg, 2); + term kind = term_get_tuple_element(msg, 3); + if (kind == READY_INPUT_ATOM) { + assert(ref_or_undefined == UNDEFINED_ATOM); + saw_read = true; + } else { + assert(kind == READY_OUTPUT_ATOM); + assert(term_is_reference(ref_or_undefined)); + saw_write = true; + } + if (i == 0) { + assert(mailbox_has_next(&ctx->mailbox)); + mailbox_next(&ctx->mailbox); + } + } + assert(saw_read); + assert(saw_write); + mailbox_remove_message(&ctx->mailbox, &ctx->heap); + mailbox_remove_message(&ctx->mailbox, &ctx->heap); + assert(!mailbox_has_next(&ctx->mailbox)); + + // Both directions have now fired and are no longer armed, so stopping the + // select should immediately release the extra refcount it was holding. + int stop_result = enif_select(env, event, ERL_NIF_SELECT_STOP, ptr, NULL, term_nil()); + assert(stop_result == ERL_NIF_SELECT_STOP_CALLED); + + int release_result = enif_release_resource(ptr); + assert(release_result); + + scheduler_terminate(ctx); + + close(pipefds[0]); + close(pipefds[1]); + +#ifdef AVM_TASK_DRIVER_ENABLED + // Notifications built without a custom message (the ref-only path + // exercised above) release their transient resource reference through + // the task-driver refc queue rather than synchronously; drain it so the + // resource is properly freed instead of merely being reported as a + // (harmless) dangling resource on shutdown. + globalcontext_process_task_driver_queues(glb); +#endif + + globalcontext_destroy(glb); +} + int main(int argc, char **argv) { UNUSED(argc); @@ -719,6 +941,8 @@ int main(int argc, char **argv) test_resource_binaries(); test_resource_release_in_down_handler(); test_resource_release_in_down_handler_two_monitors(); + test_resource_select_read_write_independent_messages(); + test_resource_select_read_write_independent_refs(); return EXIT_SUCCESS; }