Repository navigation
feat: enable TCP keepalive from the keepalive pool option - #73
Conversation
| @@ -1,5 +1,17 @@ | |||
| # ehttpc changes | |||
|
|
|||
| ## Unreleased | |||
There was a problem hiding this comment.
Bumped to 0.7.7 along with the app vsn, since 0.7.6 was tagged with #71.
| %% one the `keepalive' option would set. | ||
| with_tcp_keepalive(Timeout, GunOpts) when is_integer(Timeout), Timeout > 0 -> | ||
| %% gun's default when `tcp_opts' is not set | ||
| TCPOpts = maps:get(tcp_opts, GunOpts, [{send_timeout, 15000}, {send_timeout_close, true}]), |
There was a problem hiding this comment.
question, send_timeot and send_timeout_close is newly added.
why?
and how it behaved before ?
There was a problem hiding this comment.
Reworded the comment. It's gun's own default, not a new setting: gun:domain_lookup/3 uses [{send_timeout, 15000}, {send_timeout_close, true}] only when tcp_opts is not set, and the keepalive options set it, so the code keeps the default. On main, only a pool with transport followed by transport_opts set tcp_opts and lost it. Keepalive doesn't change that; the P2 fix does, since transport_opts in any position now sets tcp_opts.
| Idle = min(32767, max(1, Timeout div 1000)), | ||
| Interval = 5, |
There was a problem hiding this comment.
cap Interval with Idle.
Interval = min(Idle, 5)
There was a problem hiding this comment.
Done. The idle time is at least 1 second, so the interval stays between 1 and 5. Tests cover 500 ms and 3 s.
| is_gen_tcp_option({high_msgq_watermark, _}) -> true; | ||
| is_gen_tcp_option({high_watermark, _}) -> true; | ||
| is_gen_tcp_option({keepalive, _}) -> true; | ||
| %% OTP 28 and later, keepidle is Linux only |
There was a problem hiding this comment.
| %% OTP 28 and later, keepidle is Linux only | |
| %% OTP 28.3 and later, keepidle is Linux only |
There was a problem hiding this comment.
Applied, and fixed the same "OTP 28" in the tcp_keepalive_opts/2 comment and the changelog.
| with_tcp_keepalive(Timeout, GunOpts) when is_integer(Timeout), Timeout > 0 -> | ||
| %% gun's default when `tcp_opts' is not set | ||
| TCPOpts = maps:get(tcp_opts, GunOpts, [{send_timeout, 15000}, {send_timeout_close, true}]), | ||
| case lists:member({keepalive, false}, TCPOpts) of |
There was a problem hiding this comment.
[P2] Honor explicit keepalive overrides regardless of pool-option order
The opt-out is checked only after gun_opts/2 has processed the pool options, but that parser only consumes transport_opts when it comes after transport. For example, this pool-option list enables TCP keepalive despite the explicit disablement:
[{transport_opts, [{keepalive, false}]},
{transport, tcp},
{keepalive, 30_000}]I checked the live socket against both revisions: the parent has keepalive = false, while this commit has keepalive = true and an idle time of 30 seconds. The parser's order dependency predates this PR, but the new default injection now overrides the caller's requested opt-out. Extract/normalize the transport options independently of their position before adding defaults, and add a test with transport_opts before transport.
There was a problem hiding this comment.
Fixed. transport and transport_opts (first of each) are now read with proplists before the keepalive defaults, and without transport, transport_opts applies to the transport gun picks from the port. The same override happened through a proxy, where transport_opts went to the target's TLS options; its gen_tcp options now go to the connection to the proxy. Added unit cases for your list, no transport and a proxy, plus a live-socket case with transport_opts first. This also stops a lone transport and a proxy's transport from being dropped; see the commit messages.
The keepalive pool option was accepted and silently ignored. It now
enables SO_KEEPALIVE on the connection socket, with the option value
(milliseconds) as the idle time before the first probe, a 5 second
probe interval and 3 probes.
On Linux and macOS the idle time, interval and count are raw socket
options, because the native keepidle option only exists since OTP 28.3
and gen_tcp rejects it on macOS. The idle time is rounded down to
whole seconds and kept between 1 and 32767 seconds, the Linux limit.
Other systems get SO_KEEPALIVE with the OS defaults.
{keepalive, false} in transport_opts disables it. Any other keepalive
option there, native or as a raw IPPROTO_TCP option, replaces only the
matching one.
keepcnt, keepidle and keepintvl in transport_opts now go to gen_tcp.
They used to be dropped for tcp and passed to the TLS options for tls.
The native keepcnt, keepidle and keepintvl options first appear in OTP 28.3. gen_tcp raises badarg for them on earlier releases, so a connection that sets them in transport_opts fails there.
With a keepalive below 5 seconds the probe interval was longer than the idle time before the first probe.
gun_opts/2 took transport_opts only when it came after transport, and
dropped transport itself when no transport_opts followed it. With the
keepalive defaults, a {keepalive, false} in a transport_opts that came
first, or that had no transport, was overridden.
transport and transport_opts are now read with proplists, the first of
each, before the keepalive defaults are added. Without transport,
transport_opts applies to the transport gun picks from the port.
This also changes, for pool options that used to lose them:
- a transport without transport_opts is used, so {transport, tls} on a
port other than 443 connects with TLS instead of plain TCP;
- a proxy's transport and tls_opts are used for the connection to the
proxy, because parse_proxy_opts/1 puts transport_opts first;
- a transport_opts that is now used replaces gun's default send
timeouts, as one that came after transport already did.
With a proxy, parse_proxy_opts/1 passed the whole transport_opts to the
target's tls_opts, which ssl uses on the tunnel, so the gen_tcp options
there never reached a socket. The keepalive defaults went to the
connection to the proxy, the only TCP connection, and a {keepalive,
false} in transport_opts did not turn them off.
The gen_tcp options in transport_opts now go to the connection to the
proxy, before the proxy's own tls_opts, and only the TLS options are
left for the target.
When there are any, they set the proxy connection's tcp_opts, so it no
longer gets gun's default send timeouts.
20d8e2f to
6ea5b3b
Compare
|
Rebased on main and addressed the review in separate commits. |
|
tagged 0.7.7 |
2 participants
Refs emqx/emqx#19170
The
keepalivepool option was accepted but ignored. Now{keepalive, T}, whereTis a positive integer in milliseconds, adds TCP keepalive options totcp_optsfortcpandtls, with or withouttransport_opts.infinityor nokeepaliveadds nothing.On Linux and macOS it adds
{keepalive, true}and rawIPPROTO_TCPoptions for the idle time (T div 1000seconds, kept between 1 and 32767), the probe interval (5 s, or the idle time if shorter) and the probe count (3). The option numbers are 4/5/6 on Linux and 16#10/16#101/16#102 on macOS, as inemqx_schema:tcp_keepalive_opts/4. Other systems get only{keepalive, true}.{keepalive, false}intransport_optsturns it off. Any other keepalive option there, native or rawIPPROTO_TCP, replaces only the matching one.Raw options are used on every OTP version, not native ones on OTP 28.3+, because on macOS with OTP 29
gen_tcp:connect/3raisesbadargfor{keepidle, 30}, while the raw options are accepted and read back correctly.is_gen_tcp_option/1now also acceptskeepidle,keepintvlandkeepcnt. Before, they were dropped fromtransport_optsfortcpand passed to the TLS options fortls. Now they go togen_tcp, which raisesbadargfor them before OTP 28.3.transportandtransport_optsare now read withproplists(first of each) instead of in thegun_opts/2loop, which only honoredtransport_optsaftertransportand dropped atransportnot followed bytransport_opts. Withouttransport,transport_optsapplies to the transport gun picks from the port. So a lone{transport, tls}on a port other than 443 now uses TLS, and a proxy that setstransportnow uses it and itstls_optsfor the connection to the proxy. Atransport_optsthat used to be ignored now setstcp_opts, so those pools lose gun's default send timeouts, as pools withtransportthentransport_optsalready did.With a
proxy, thegen_tcpoptions intransport_optsnow go to the connection to the proxy, the only TCP connection, and only the TLS options go to the target. Before, all of them went to the target'stls_opts, so{keepalive, false}there left the keepalive defaults on the proxy connection. If there are any, they set the proxy connection'stcp_opts, so it loses gun's default send timeouts.emqx always passes
transportbeforetransport_opts, and its onlyproxy(Snowflake) sets justhostandportwith TLS-onlytransport_opts, so it is not affected.App vsn bumped to 0.7.7.
Tests
tcp_keepalive_opts_test_andtcp_keepalive_gun_opts_test_insrc/ehttpc.erl: the mapping per OS, the 1 and 32767 second limits, the interval cap,tcp,tlsand notransport_opts, and keepalive options set by the caller.transport_gun_opts_test_insrc/ehttpc.erl:transport_optsbeforetransport, notransport, a lonetransport, and a proxy withtransportandtls_opts.proxy_tcp_opts_test_insrc/ehttpc.erl: with a proxy, thegen_tcpoptions and{keepalive, false}go to the connection to the proxy, and the TLS options to the target.tcp_keepalive_test_intest/ehttpc_tests.erl: sends a request through a pool with{keepalive, 30_000}and readskeepaliveand the raw idle, interval and count back from the live socket, with and withouttransport_opts, and checks that{keepalive, false}beforetransportleaves keepalive off.Reverting the interval cap, the
transporthandling or the proxy split fails the new cases. On OTP 27 and 28 on Linux,make fmt-check,make compile,make xrefandmake dialyzerpass, andmake eunitpasses 113/113 with tinyproxy. OTP 25/26 are left to CI.