Hello Willy,
On Sun, Apr 3, 2016 at 11:15 PM, Willy Tarreau <[email protected]> wrote:
> On Thu, Mar 31, 2016 at 12:37:03PM -0700, Frederik Deweerdt wrote:
> > >> It seems that we would be a bit more efficient if we also aborted when
> > >> si_b->state was SI_ST_INI: that is, don't even try to open a connection
> > >> to the backend if we're shutting down the frontend.
> > >
> > > No, we should not do this. You can already force this behaviour with
> > > "option abortonclose".
> >
> > Mmm, adding "option abortonclose" does work in "mode http", but not in
> > "mode tcp", which I've been using.
>
> Why are you saying this ?
That's what I'm seeing in my tests, with ssl_sock_to_buf instrumented to
fail the second time the function is called. With "option abortonclose":
- mode http -> closes the frontend connection
- mode tcp -> tries to forward the request (and the stale fd reaches
conn_si_send_proxy()), and then closes the frontend connection
> I'm not seeing anything restricting abortonclose
> to HTTP mode only.
I wasn't able to explain the difference between tcp and http either,
so I've done some further tests:
As we know, for the issue to happen, we need two conditions: we must
have read bits from the request and we must have a read error. My test
setup instruments ssl_sock_to_buf() so that we get an error on the
second call to it, so that these conditions are true.
In tcp mode, when we first reach si_conn_recv_cb(), we:
- call conn->xprt->rcv_buf() (aka. the aforementioned ssl_sock_to_buf)
- the first read works fine
- we optimistically perfom a second read, this times that returns an
error, we exit
- we go on opening the connection to the backend
In http mode, we first reach si_conn_recv_cb():
- call conn->xprt->rcv_buf()
- the first read works fine
- we exit the function *because CF_READ_DONTWAIT is set*
> > I can however confirm that if the
> > check becomes:
> > (s->be->options & PR_O_ABRT_CLOSE || channel_is_empty(req))
> > rather than
> > channel_is_empty(req)
> > it does close the connection there.
>
> Then you definitely need to enable the option :-)
>
Yes, I understand now that this is the way to go. It does seem that
having something along the patch below is needed to prevent the connection
to the backend to be opened in tcp mode.
diff --git a/src/stream.c b/src/stream.c
index 4d87fc9..cf70171 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -2185,7 +2185,7 @@ struct task *process_stream(struct task *t)
/* shutdown(write) pending */
if (unlikely((req->flags & (CF_SHUTW|CF_SHUTW_NOW)) == CF_SHUTW_NOW &&
- channel_is_empty(req))) {
+ (s->be->options & PR_O_ABRT_CLOSE || channel_is_empty(req)))) {
if (req->flags & CF_READ_ERROR)
si_b->flags |= SI_FL_NOLINGER;
si_shutw(si_b);
Regards,
Frederik
