Hi Matthias, On Sun, Mar 12, 2017 at 09:34:31AM +0100, Matthias Fechner wrote: > Hi Willy, > > Am 11.03.2017 um 14:13 schrieb Willy Tarreau: > > > > OK so this is the one that I initially suspected and that after you > > reverted, didn't fix the issue for you. > > > > Are you sure you didn't have a problem when you reverted it ? (eg: > > failed to restart the process or something like this). > > > > At least now we're certain it's this one so we can try to imagine > > any possible corner case or any remaining bug this one could have > > uncovered. > > I tested it again with the patch and I get the result: > patch -Rp1 > </home/idefix/0001-BUG-MEDIUM-tcp-don-t-poll-for-write-when-connect-suc.patch > ... > |--- > | src/proto_tcp.c | 30 +++++++++++++++++++++++++----- > | 1 file changed, 25 insertions(+), 5 deletions(-) > | > |diff --git a/src/proto_tcp.c b/src/proto_tcp.c > |index f6d8ca1..c04f276 100644 > |--- a/src/proto_tcp.c > |+++ b/src/proto_tcp.c > -------------------------- > Patching file src/proto_tcp.c using Plan A... > Hunk #1 failed at 474. > Hunk #2 failed at 514. > Hunk #3 failed at 523. > Hunk #4 failed at 531. > 4 out of 4 hunks failed--saving rejects to src/proto_tcp.c.rej > Hmm... Ignoring the trailing garbage. > done > > > What I understand from the output is that the revert got rejected in > src/proto_tcp.c. > So that explains why the test failed.
Yep, that totally makes sense. Thanks for checking. Please find in attachment one which does properly apply here with -Rp1 (at least it will allow you to fix your production for the time it takes to find the root cause of this issue). If it doesn't apply it may mean that you have some leftovers of a previous patch or that your tree has other local patches, which could also explain the problem. > I checked the port again and there is one patch applied to haproxy, but > it is a different file, so it should not cause the patch to fail, but > maybe can cause other problems. > --- src/hlua_fcn.c.orig 2016-12-17 13:58:44.786067000 +0300 > +++ src/hlua_fcn.c 2016-12-17 13:59:17.551256000 +0300 > @@ -39,6 +39,12 @@ static int class_listener_ref; > > #define STATS_LEN (MAX((int)ST_F_TOTAL_FIELDS, (int)INF_TOTAL_FIELDS)) > > +#if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) > +#define s6_addr8 __u6_addr.__u6_addr8 > +#define s6_addr16 __u6_addr.__u6_addr16 > +#define s6_addr32 __u6_addr.__u6_addr32 > +#endif > + No, it's harmless, it's just a build fix. Best regards, Willy
>From cd4c5a3ecf5e77fb4734c423c914f7280199c763 Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Wed, 25 Jan 2017 14:12:22 +0100 Subject: BUG/MEDIUM: tcp: don't poll for write when connect() succeeds X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 While testing a tcp_fastopen related change, it appeared that in the rare case where connect() can immediately succeed, we still subscribe to write notifications on the socket, causing the conn_fd_handler() to immediately be called and a second call to connect() to be attempted to double-check the connection. In fact this issue had already been met with unix sockets (which often respond immediately) and partially addressed but incorrect so another patch will follow. But for TCP nothing was done. The fix consists in removing the WAIT_L4_CONN flag if connect() succeeds and to subscribe for writes only if some handshakes or L4_CONN are still needed. In addition in order not to fail raw TCP health checks, we have to continue to enable polling for data when nothing is scheduled for leaving and the connection is already established, otherwise the caller will never be notified. This fix should be backported to 1.7 and 1.6. (cherry picked from commit 819efbf4b532d718abeb5e5aa6b2521ed725fe17) --- src/proto_tcp.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/proto_tcp.c b/src/proto_tcp.c index f6d8ca1..c04f276 100644 --- a/src/proto_tcp.c +++ b/src/proto_tcp.c @@ -474,10 +474,16 @@ int tcp_connect_server(struct connection *conn, int data, int delack) if (global.tune.server_rcvbuf) setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &global.tune.server_rcvbuf, sizeof(global.tune.server_rcvbuf)); - if ((connect(fd, (struct sockaddr *)&conn->addr.to, get_addr_len(&conn->addr.to)) == -1) && - (errno != EINPROGRESS) && (errno != EALREADY) && (errno != EISCONN)) { - - if (errno == EAGAIN || errno == EADDRINUSE || errno == EADDRNOTAVAIL) { + if (connect(fd, (struct sockaddr *)&conn->addr.to, get_addr_len(&conn->addr.to)) == -1) { + if (errno == EINPROGRESS || errno == EALREADY) { + /* common case, let's wait for connect status */ + conn->flags |= CO_FL_WAIT_L4_CONN; + } + else if (errno == EISCONN) { + /* should normally not happen but if so, indicates that it's OK */ + conn->flags &= ~CO_FL_WAIT_L4_CONN; + } + else if (errno == EAGAIN || errno == EADDRINUSE || errno == EADDRNOTAVAIL) { char *msg; if (errno == EAGAIN || errno == EADDRNOTAVAIL) { msg = "no free ports"; @@ -514,6 +520,10 @@ int tcp_connect_server(struct connection *conn, int data, int delack) return SF_ERR_SRVCL; } } + else { + /* connect() == 0, this is great! */ + conn->flags &= ~CO_FL_WAIT_L4_CONN; + } conn->flags |= CO_FL_ADDR_TO_SET; @@ -523,7 +533,6 @@ int tcp_connect_server(struct connection *conn, int data, int delack) conn_ctrl_init(conn); /* registers the FD */ fdtab[fd].linger_risk = 1; /* close hard if needed */ - conn_sock_want_send(conn); /* for connect status */ if (conn_xprt_init(conn) < 0) { conn_force_close(conn); @@ -531,6 +540,17 @@ int tcp_connect_server(struct connection *conn, int data, int delack) return SF_ERR_RESOURCE; } + if (conn->flags & (CO_FL_HANDSHAKE | CO_FL_WAIT_L4_CONN)) { + conn_sock_want_send(conn); /* for connect status, proxy protocol or SSL */ + } + else { + /* If there's no more handshake, we need to notify the data + * layer when the connection is already OK otherwise we'll have + * no other opportunity to do it later (eg: health checks). + */ + data = 1; + } + if (data) conn_data_want_send(conn); /* prepare to send data if any */ -- 2.8.0.rc2.1.gbe9624a
