Hi Frederik,
On Wed, Apr 06, 2016 at 08:49:09AM -0700, Frederik Deweerdt wrote:
> > > 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
OK, thanks for the precisions.
> > 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*
So this means that in TCP mode we're aware of the abort earlier than in
HTTP mode. Thus we theorically have everything needed to decide not to
connect if possible.
> > > 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);
This one will result in truncated transfers when shutting down at the end
so we must not do it this way, but I see your point. I guess one reason
why TCP's abort is not caught is that we don't enter process_stream() once
the connection is established and we don't have any more analysers (which
is true in TCP). In HTTP we have several opportunities to get back there
and possibly to abort the connection early.
I think that we'd need to add tests for PR_O_ABRT_CLOSE before deciding
to establish a connection, it's done in sess_update_stream_int(), for
state SI_ST_ASS. In certain states such as SI_ST_QUE (queue) or SI_ST_TAR
(turn-around, wait before retrying), the test is already performed, but
its missing here before the call to connect_server(). Do you want to try
this instead ? If it works, simply send a patch with a description
according to the CONTRIBUTING file and I'll happily merge it.
Thanks!
Willy
