feat!: the socket transport by default, and options that say when they stop - #45
Merged
Conversation
…y stop `transport` defaults to `socket` rather than `inet`. It does everything the driver does on every machine minato runs on, and the saturation bench says where that is worth having: nothing at sixteen connections, where the pool is the ceiling before the client is, and about a fifth more queries a second for about a sixth less of the machine at sixty-four. `transport => inet` is the way back. A transport that is chosen by default has to answer for the options handed to it. `minato_socket:setopts/2` set `nodelay` and dropped everything else on the floor, which was survivable while it was opt in and is not now: `keepalive` on a connection through a firewall would have quietly become nothing. It now sets `nodelay`, `keepalive`, `recbuf` and `sndbuf`, accepts and ignores the ones it always does or has no equivalent of, and refuses anything else at connect rather than silently dropping it. The socket suite becomes `minato_transport_SUITE` and runs every case against both transports as groups. Before this the rest of the suite covered the default and the socket suite covered the other one; leaving it alone would have swapped which transport was tested nowhere.
🟡 Code Coverage — 88.8%1724 of 1941 lines covered. ✅ ELP LintNo diagnostics. |
Windows found this the moment the socket transport became the default and the
listener started running over it. A peer that closes while a read is
outstanding does not answer the completion with an error there - it aborts it,
and `{'$socket', _, abort, _}` fell through `handle_message/2` to `ignore`. The
owner then waited for a message that was never coming, which is a listener that
never reconnects. `activate/1` on the same socket afterwards raised
`{invalid, state}` out of `prim_socket:nif_recv/4` rather than answering, which
is the listener crashing instead.
`abort` is now a close for both shapes of wait, and `minato_socket:activate/1`
answers `{error, closed}` for a socket with no state left to read in.
The case is in the transport suite for both transports, because the three ways
this can be said - a `select` that never comes, a completion answered with an
error, an abort - are one invariant: an owner waiting on a connection that died
has to hear about it.
`minato_listener` re-activates whenever anything about its subscribers changes:
a second subscriber to a channel already listened to, a subscriber going down,
an unsubscribe. On the driver that is `{active, once}` set twice and costs
nothing. On a socket handle it is a second read on the same handle, which a
completion system refuses outright - the Windows run raised `{invalid, state}`
out of `prim_socket:nif_recv/4` and took the listener down with it.
`activate/1` on a connection that is already waiting now answers with the
connection. One wait is what both calls asked for.
The case asserted the close arrived as a message, but arming a socket the
server has already gone from answers `{error, {socket, closed}}` instead, and
which of the two happens is a race with the server's own shutdown - OTP 29 took
the other branch and the case called it a failure. Both are the connection
saying it closed, which is the whole of what the case is for.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
transportdefaults tosocketrather thaninet.Why now
The saturation bench answered the question the per-operation numbers could not. At sixteen connections it changes nothing - every pooled client plateaus around 16k q/s and the pool is the ceiling before the client is. At sixty-four it is about a fifth more queries a second for about a sixth less of the machine. The transport does everything the driver does on every machine minato runs on, Windows included, and the Windows workflow proves it.
transport => inetis one option away for a machine or a workload that disagrees.What else had to change
minato_socket:setopts/2setnodelayand silently dropped everything else. That was survivable while the transport was opt in. As the default it is a defect:keepaliveon a connection through a firewall would have quietly become nothing. It nownodelay,keepalive,recbufandsndbuf,binary,{mode, binary},{packet, raw},{packet, 0},{active, false}andbuffer, which it always does or has no equivalent of,The moduledoc already promised that refusal. The code did not do it.
Coverage
minato_socket_SUITEbecomesminato_transport_SUITEand runs all fourteen cases against both transports as groups. Before this the rest of the suite covered whatever the default was and the socket suite covered the other one, so flipping the default without this would have moved which transport is tested nowhere rather than fixing it. Two new cases cover the option handling from both sides: options the driver takes are taken here, and one it cannot honour is refused.Breaking
socket_optionoutside the list above now fails at connect on the default transport instead of connecting without it. That is the intent - it was already not being applied.bench/README.mdper-operation table is annotated with the transport it was measured on;minato_benchtakes the default, so a run made now measuressocket.Checks
fmt --check,xref,dialyzer,elp eqwalize-all,elp lint,ex_docclean. 1398 eunit, 169 ct including both transport groups with TLS andLISTENover each.