Repository navigation
Conversation
|
thank you for pushing the httpd side! this is obviously correct - however it does increase the leakage of underlying implementation, and now any caller will also need to implement their own unique retry mechanism (gen_tcp_socket.erl, socket_dist_controller.erl, ssl.erl - and atomvm_lib httpd) - and is it 5ms with exponential backoff or xyz - and we end up with complexity, duplicated and slow (sleeping) code.. I believe the solution is to implement nif_select_write/2, with proper write-select and no sleeping/guessing, eg. handle the happy path out of the box, similar to otp: see https://github.com/petermm/AtomVM/tree/nif_select_write/2 (based on this PR) |
|
Thanks @petermm! I agree, that sounds like a better approach. Should we continue from your branch, or should i incorporate those changes here? |
|
@petermm Thanks for the review and for prototyping this on
Verified: One thing worth calling out from writing the test: I initially had it drain only a small chunk (64KB) of the peer's backlog and expected the socket to become writable again, but that hung — traced it down (gdb backtraces + an independent raw C |
harmon25
changed the title
harmon25
changed the title
harmon25
changed the title
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
First of all I apologize for the long wait before this review. Here I have some comments.
I would like to ask you make your branch git history linear without those merges from release-0.7. Ideally your commits should be amended using git commit --fixup and they should sit on top of latest release-0.7. See also GIT_WORKFLOW_AND_HISTORY_STYLE_GUIDE.md
Blocking sends require read and write readiness to coexist on one socket. Sharing their owner lets a writer replace a reader, misroutes close notifications, and cancels both directions when one selector exits. Give each direction its own owner, monitor and reference, initialized for both opened and accepted sockets. Implement ERL_NIF_SELECT_CANCEL so selector death cancels only its direction, disposes its pending message, and leaves the other direction armed. Retain socket resources while lwIP write/error events are queued and detach callbacks on close. Notify established-socket waiters of terminal errors through direction-specific abort messages, independently of the connect trap. Keep distribution output event-driven so backpressure cannot block incoming traffic or ticks. Preserve pending frame suffixes and queue ticks behind them to maintain byte ordering. Send empty binaries through the native socket path so closed sockets still fail and connected UDP sockets transmit zero-length datagrams. Add regressions for accepted-socket close, writer death, both selector registration orders, exact-data backpressure recovery, cancellation cleanup, and empty UDP sends. Move the changelog entry to beta.1. See also: atomvm#2345 Signed-off-by: Peter M <petermm@gmail.com>
Blocking sends require read and write readiness to coexist on one socket. Sharing their owner lets a writer replace a reader, misroutes close notifications, and cancels both directions when one selector exits. Give each direction its own owner, monitor and reference, initialized for both opened and accepted sockets. Implement ERL_NIF_SELECT_CANCEL so selector death cancels only its direction, disposes its pending message, and leaves the other direction armed. Retain socket resources while lwIP write/error events are queued and detach callbacks on close. Notify established-socket waiters of terminal errors through direction-specific abort messages, independently of the connect trap. Keep distribution output event-driven so backpressure cannot block incoming traffic or ticks. Preserve pending frame suffixes and queue ticks behind them to maintain byte ordering. Send empty binaries through the native socket path so closed sockets still fail and connected UDP sockets transmit zero-length datagrams. Add regressions for accepted-socket close, writer death, both selector registration orders, exact-data backpressure recovery, cancellation cleanup, and empty UDP sends. Move the changelog entry to beta.1. See also: atomvm#2345 Signed-off-by: Peter M <petermm@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
Let's fix the CodeQL error and it is good for merge.
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
I need to apologize, I think I found other issues just before merging.
|
Pushed fixes for all six comments, rebased on the latest |
Transient send backpressure (lwIP ERR_MEM, BSD EAGAIN/EWOULDBLOCK) fell
through to the generic error branch of nif_socket_send_internal and was
reported as {error, closed}, so callers could not tell a full send
buffer from a peer that went away. Conversely, an actually closed
connection was reported as {ok, Data}, as if nothing had been consumed.
Report backpressure as {error, eagain} and a closed connection as
{error, closed}.
Signed-off-by: harmon25 <dougwright1@gmail.com>
enif_select_write was declared in erl_nif.h but never implemented, and ERL_NIF_SELECT_CANCEL was documented as unimplemented. A select event used to hold a single ref, message and pid shared by the read and the write direction, so selecting one direction silently overwrote the other. Keep them independent, so a resource can have a pending read select and a pending write select at the same time, each with its own owner. Implement ERL_NIF_SELECT_CANCEL (with the *_CANCELLED result flags) to cancel a single direction while keeping the event and the other direction armed. Also fix the ref ticks computation, that tested ref against UNDEFINED_ATOM instead of checking it is a local reference. Signed-off-by: harmon25 <dougwright1@gmail.com>
Add a write select to the socket NIFs, so a process can wait until a
socket can accept more data instead of polling after {error, eagain}.
BSD sockets use enif_select_write. lwIP sockets use the tcp_sent and
tcp_poll callbacks, and tcp_sndbuf/tcp_sndqueuelen to tell when the send
buffer has room again.
A socket has one selecting process and monitor per direction, like
resources.c: selecting in one direction replaces the previous owner of
that direction only, close and select_stop handle both, and socket_down
releases only the direction of the dead process. A reader and a writer
can therefore block on the same socket without disturbing each other,
and an abort is sent to the owner of each direction. On lwIP the
callbacks notify the owner of their direction, and a connect traps its
caller on the write slot.
The selecting state is initialized by a helper shared by all the ways a
socket is created, including accept.
Signed-off-by: harmon25 <dougwright1@gmail.com>
Make socket:send/2 and socket:sendto/3 retry partial sends and
{error, eagain} internally, waiting on nif_select_write/2 for the socket
to become writable again. Callers get ok | {error, Reason} and do not
need their own retry and backoff loop.
Signed-off-by: harmon25 <dougwright1@gmail.com>
socket:send/2 no longer returns {ok, Rest} or {error, eagain}.
- ssl: wait for write readiness in the handshake, close notify, send and
recv loops instead of busy looping on want_write.
- socket_dist_controller: terminate the connection when socket:send/2
fails on a tick or on outgoing data, instead of ignoring the error.
- Remove the {ok, Rest} branches that can no longer happen from epmd and
the examples.
Signed-off-by: harmon25 <dougwright1@gmail.com>
- A write select on a socket with a full send buffer wakes up when the peer drains it, and socket:send/2 then completes transparently. - Closing an accepted socket selected by another process. It would have caught accepted sockets not initializing their selecting state, under valgrind. - A process blocked in recv and another blocked in send on the same socket are both aborted when a third process closes it. - When the process blocked in send dies, the one blocked in recv still receives data. Signed-off-by: harmon25 <dougwright1@gmail.com>
Signed-off-by: harmon25 <dougwright1@gmail.com>
|
Pushed fixes for all six comments, rebased on the latest |
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
We are getting close to green light, but there are still some issues.
| {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 |
There was a problem hiding this comment.
Let's avoid narration comment, such this one.
| // after tcp_shutdown succeeded so that a failed shutdown leaves the | ||
| // socket intact, and under the lwIP lock so no callback can run in | ||
| // between. | ||
| socket_tcp_detach_callbacks(rsrc_obj->tcp_pcb, rsrc_obj->socket_state); |
There was a problem hiding this comment.
tcp_shutdown(pcb, 1, 1) is lwIP's tcp_close_shutdown(pcb, 1). It frees the pcb and still returns ERR_OK when the pcb is in CLOSED or SYN_SENT state, and in ESTABLISHED/CLOSE_WAIT whenever rcv_wnd != TCP_WND_MAX or refused_data is set (the RST path).
Since tcp_recved only runs when data is copied out in recv (line 2310), any shutdown(S, read_write) with unread data takes that path, and socket_tcp_detach_callbacks then writes callback_arg, recv, sent, poll and err into freed memory.
Detach before tcp_shutdown, as socket_dtor and nif_socket_close do. The only non-freeing failure is ERR_MEM from tcp_close_shutdown_fin, which sets TF_CLOSEPEND so lwIP finishes the close itself; keeping the callbacks detached in that case is fine.
| %% Receives at least Remaining bytes (a recv can return more than needed). | ||
| recv_at_least(_Socket, Remaining) when Remaining =< 0 -> | ||
| ok; | ||
| recv_at_least(Socket, Remaining) -> |
There was a problem hiding this comment.
{recv_failed,timeout,69620736} is thrown here.
Remaining is larger than ?LARGE_PAYLOAD_SIZE, so the server read fewer bytes than Sent, i.e. less than the client kernel had already accepted, then saw nothing for 10 s. That is not writer slowness: queued kernel data cannot stall, so it points at a lost read-select wakeup or an accounting bug specific to that build. The job passed on e2e3ec3 and fails identically on 42a027c and c34bb11, so it tracks the nif_select_read_stop round.
I could not reproduce on 64-bit (native, pinned to one core, valgrind, valgrind --fair-sched=yes); it needs a 32-bit run with the test instrumented before merge.
| LWIP_END(); | ||
| if (can_send) { | ||
| // Clear flag to avoid sending a message again. | ||
| rsrc_obj->socket_state &= ~SocketStateSelectingWrite; |
There was a problem hiding this comment.
nif_socket_select updates socket_state under LWIP_BEGIN/END (lines 1283, 1286 and the read branch), while this handler does a read-modify-write of the same word with no lock.
RP2 builds are SMP by default, so a reader doing |= SelectingRead on one core while this clears the write bit on the other loses a bit: the reader is never notified, or a second write notification goes to a consumed ref. tcp_recv_handler has the same pre-existing pattern, but two processes selecting both directions is now the normal case. Move the flag update and the write_select.process_id read inside the locked region.
| if (LIKELY(rsrc_obj) && (rsrc_obj->socket_state & SocketStateSelectingWrite)) { | ||
| struct LWIPEvent event; | ||
| event.handler = tcp_sent_handler; | ||
| event.tcp_sent.rsrc_obj = rsrc_obj; |
There was a problem hiding this comment.
The queued event carries a raw rsrc_obj. Between the enqueue in lwIP context and tcp_sent_handler, socket_down can clear the flag and drop the owner's reference, which can be the last one, so the handler dereferences freed memory. tcp_recv_cb avoids this by queuing tpcb and reading tpcb->callback_arg, which socket_tcp_detach_callbacks NULLs under the lock. Do the same here (also at line 1145), or enif_keep_resource in the callback and release in the handler.
| Total = recv_until_closed(ServerSocket, 0), | ||
| Owner ! {drained_all, Total}, | ||
| ok = socket:close(ServerSocket) | ||
| after 60000 -> |
There was a problem hiding this comment.
Under valgrind on a loaded runner, draining 69 MB can exceed this, and the close from another process turns a slow run into a confusing {recv_failed, closed, _} or an abort on the writer. Teardown always sends drain_all, so use infinity or a much larger value.
| Reader = spawn(fun() -> | ||
| Self ! {recv_result, self(), socket:recv(Client, 0, 30000)} | ||
| end), | ||
| timer:sleep(100), |
There was a problem hiding this comment.
Lines 913 and 921 assume the reader and writer are blocked after 100/200 ms, and test_send_error_after_partial_send (lines 871, 876) assumes the writer made progress within 200 ms of the drain, otherwise it gets a plain {error, closed} and fails. Under valgrind on slow CI these are flakiness sources. Have the writer report when it has observed eagain (e.g. fill via raw nif_send in the writer itself before socket:send), and retry the drain until a partial send is observed.
| RAISE_ERROR(OUT_OF_MEMORY_ATOM); | ||
| } | ||
| } | ||
| if ((rsrc_obj->socket_state & SocketStateSelectingWrite) |
There was a problem hiding this comment.
This notifies the process selecting for write, but SocketStateSelectingWrite is only cleared by the SocketStateClosed store at line 938 and tcp_pcb only at line 930, both after the LWIP_END() at line 925.
tcp_sent_handler does not take socket_lock, so on an SMP build an event already queued by tcp_sent_cb/tcp_poll_cb can pass the SocketStateSelectingWrite check at line 1099 in that window and call lwip_tcp_can_send under the lwIP lock at line 1103, reading snd_buf of a pcb that tcp_close may already have freed (for example in CLOSED or SYN_SENT, or on the RST path with unread data).
Please clear tcp_pcb and set the closed state inside the locked block at lines 922-925, as socket_dtor does for the pcb at line 326.
| ok | {error, Reason :: term()}. | ||
| sendto(Socket, Data, Dest) when is_binary(Data) -> | ||
| ?MODULE:nif_sendto(Socket, Data, Dest); | ||
| sendto_all(Socket, Data, Dest); |
There was a problem hiding this comment.
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.
| recv_until_closed(Socket, Acc + byte_size(Data)); | ||
| {error, closed} -> | ||
| Acc; | ||
| {error, timeout} -> |
There was a problem hiding this comment.
{error, timeout} ends the drain quietly with a partial count that no caller checks (they only match is_integer(N)), and the acceptor then closes the server socket. In a healthy run a recv never waits 30 s here, since the client is either still sending or has already closed, so this only fires when the drain has stalled.
If that happens while test_send_backpressure is still in socket:send/2 at line 647, the failure shows up there (a badmatch on {error, closed} or {error, {closed, Rest}}, or a hang) instead of as the stalled drain it is. Since the acceptor is linked, raising here as recv_at_least does at line 901 would keep such a failure diagnosable.
| // Another process selecting the same direction is replaced, not queued. | ||
| struct SocketSelect *select = socket_get_select(rsrc_obj, direction); | ||
| if (select->process_id != ctx->process_id) { | ||
| socket_select_disown(env, rsrc_obj, select); |
There was a problem hiding this comment.
On lwIP, nif_socket_connect parks the process trapped in connect in the write slot, without a monitor (line 3078). A write select from another process while the socket is still connecting reaches this line, disowns that process and takes the slot, and only then raises badarg in the lwIP branch (line 1298): the connecting process is never answered. Please reject a write select on a SocketStateTCPNew socket before this point (patch 0009 does that).
|
Few additional patches (that you can use as a fix suggestions) for this PR. 0018-gen_udp_socket-Fix-send-blocking-the-server.patch |
Blocking sends require read and write readiness to coexist on one socket. Sharing their owner lets a writer replace a reader, misroutes close notifications, and cancels both directions when one selector exits. Give each direction its own owner, monitor and reference, initialized for both opened and accepted sockets. Implement ERL_NIF_SELECT_CANCEL so selector death cancels only its direction, disposes its pending message, and leaves the other direction armed. Retain socket resources while lwIP write/error events are queued and detach callbacks on close. Notify established-socket waiters of terminal errors through direction-specific abort messages, independently of the connect trap. Keep distribution output event-driven so backpressure cannot block incoming traffic or ticks. Preserve pending frame suffixes and queue ticks behind them to maintain byte ordering. Send empty binaries through the native socket path so closed sockets still fail and connected UDP sockets transmit zero-length datagrams. Add regressions for accepted-socket close, writer death, both selector registration orders, exact-data backpressure recovery, cancellation cleanup, and empty UDP sends. Move the changelog entry to beta.1. See also: atomvm#2345 Signed-off-by: Peter M <petermm@gmail.com>
Distinguish transient send backpressure from a closed connection in
socket:send/2,3, and wait for write-readiness with a proper write select instead of leaking{error, eagain}or{ok, Rest}to callers.Ran into this building httpd, a fork/standalone implementation of
atomvm_lib/httpd: large or many subsequent requests failed with the socket closing early, because lwIPERR_MEMbackpressure was reported as{error, closed}.Commits
ERR_MEM/ BSDEAGAIN|EWOULDBLOCKis now{error, eagain}, and a closed connection is{error, closed}instead of{ok, Data}.enif_select_writewas declared but never implemented. A select event now keeps independent read and write state (ref, message, pid), so a resource can have a pending read and a pending write select at the same time.ERL_NIF_SELECT_CANCEL(with the*_CANCELLEDflags) cancels one direction and keeps the other armed. Also fixes the ref ticks computation. C tests intests/test-enif.c.enif_select_write; lwIP usestcp_sent/tcp_pollandtcp_sndbuf/tcp_sndqueuelen. A socket has one selecting process and monitor per direction, likeresources.c: select replaces only its own direction, close and select_stop handle both,socket_downreleases only the dead owner's direction (and no longer aborts the lwIP connection, like BSD), aborts go to each direction's own pid, and the lwIP handlers notify the owner of their direction. The selecting state is initialized by one helper shared by open and accept.socket:nif_select_read_stop/1stops only the read direction when another process is selecting for write, and is used by the read/accept error cleanup paths so that a failed read does not disown a blocked writer. On lwIP,tcp_sentevents are only queued while a write select is pending,lwip_tcp_can_sendis called with the lwIP lock held and only on connected sockets, and every pcb callback is detached before a pcb is closed so none can run against a freed resource.socket:send/2andsendto/3wait onnif_select_write/2and retry, so callers getok | {error, Reason}(or{error, {Reason, RestData}}after a partial send, as in OTP); empty payloads still go through the NIF like before.sslwaits for write-readiness instead of busy looping onwant_write;socket_dist_controllerterminates the connection whensocket:send/2fails; dead{ok, Rest}branches are removed.Known limitations
tcp_err_cb) does not wake a process blocked insocket:send/2orrecv. It only answers a pending connect, as before. Tracked in lwIP sockets: a peer reset does not wake processes blocked in socket:send/2 or recv #2432.socket:send/2now blocks, sosocket_dist_controllercan block on a peer that stops reading and miss the tick that would detect it: socket_dist_controller: sends can block the controller and hide a dead peer #2424.Testing
test-enif(including the newERL_NIF_SELECT_CANCELtests) and thetest_estdlibsuite on generic_unix, includingtest_tcp_socketunder valgrind.test_serial_dist_socatfails in the same way on a plainrelease-0.7build here (socat/pty environment).nif_select_read_stop), and emptysend/2/sendto/3payloads (also run on BEAM).test-enifpasses on each. ESP32 (ESP-IDF 5.5.4) build, and the lwIP code path syntax-checked against ESP-IDF's lwIP headers: not run on RP2 hardware.atomvm_httpd: 3 concurrent slow readers (curl --limit-rate 1kagainst a 64KB response) logged onlyeagainretries (neverclosed) and every response completed with the full body. That run predates the review changes below.These changes are made under both the "Apache 2.0" and the "GNU Lesser General Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later