EOS is now acknowledge at the end of sc_conn_recv(), even if an error was
encountered. There is no reason to not do so, especially because, if it not
performed here, it will be ack in sc_conn_process().
Note, it is still performed in sc_conn_process() because this function is
also the .wake callback function and can be directly called from the lower
layer.
In sc_conn_recv(), if the EOS is reported by the endpoint, it will always be
acknowledged by the SC and a read0 will be performed on the input
channel. Thus there is no reason to still test at the begining of the
function because there is already a test on CF_SHUTR.
In the commut b08c5259e ("MINOR: stconn: Always report READ/WRITE event on
shutr/shutw"), a return statement was erroneously removed from
sc_app_shutr(). As a consequence, CF_SHUTR flags was never set. Fortunately,
it is the default .shutr callback function. Thus when a connection or an
applet is attached to the SC, another callback is used to performe a
shutdown for reads.
It is a 28-dev specific issue. No backport needed.
If we fail to allocate a new stream in sc_new_from_endp(), and the call
to sc_new() allocated the sedesc itself (which normally doesn't happen),
then it doesn't get released on the failure path. Let's explicitly
handle this case so that it's not overlooked and avoids some head
scratching sessions.
This may be backported to 2.6.
There's an occasional crash that can be triggered in sc_detach_endp()
when calling conn->mux->detach() upon memory allocation error. The
problem in fact comes from sc_attach_mux(), which doesn't reset the
sc type flags upon tasklet allocation failure, leading to an attempt
at detaching an incompletely initialized stconn. Let's just attach
the sc after the tasklet allocation succeeds, not before.
This must be backported to 2.6.
Instead of reporting a blocked send if nothing is send, we do it if some
output data remain blocked after a write attempts or after a call the the
applet's I/O handler. It is mandatory to properly handle write timeouts.
Indeed, if an endpoint is blocked for a while but it partially consumed
output data, no timeout is triggered. It is especially true for
connections. But the same may happen for applet, there is no reason.
Of course, if the endpoint decides to partially consume output data because
it must wait to move on for any reason, it should use the se/applet API
(se/applet_will_consume(), se/applet_wont_consume() and
se/applet_need_more_data()).
This bug was introduced during the channels timeouts refactoring. No
backport is needed.
The half-closed timeout is now directly retrieved from the proxy
settings. There is no longer usage for the .hcto field in the stconn
structure. So let's remove it.
We now directly use the proxy settings to set the half-close timeout of a
stream-connector. The function sc_set_hcto() must be used to do so. This
timeout is only set when a shutw is performed. So it is not really a big
deal to use a dedicated function to do so.
It was done by hand by callers when a shutdown for read or write was
performed. It is now always handled by the functions performing the
shutdown. This way the callers don't take care of it. This will avoid some
bugs.
The stream endpoint descriptor now owns two date, lra (last read activity) and
fsb (first send blocked).
The first one is updated every time a read activity is reported, including data
received from the endpoint, successful connect, end of input and shutdown for
reads. A read activity is also reported when receives are unblocked. It will be
used to detect read timeouts.
The other one is updated when no data can be sent to the endpoint and reset
when some data are sent. It is the date of the first send blocked by the
endpoint. It will be used to detect write timeouts.
Helper functions are added to report read/send activity and to retrieve lra/fsb
date.
Read and write timeouts (.rto and .wto) are now replaced by an unique
timeout, call .ioto. Since the recent refactoring on channel's timeouts,
both use the same value, the client timeout on client side and the server
timeout on the server side. Thus, this part may be simplified. Now it
represents the I/O timeout.
After I/O handling, in sc_notify(), the stream's task is no longer
requeue. The stream may be woken up. But its task is not requeue. It is
useless nowadays and only avoids a call to process_stream() for edge
cases. It is not really a big deal if the stream is woken up for nothing
because its task expired. At worst, it will be responsible to compute its
new expiration date.
These timers are related to the I/O. Thus it is logical to move them into
the SE descriptor. The patch is a bit huge but it is just a
replacement. However it is error-prone.
From the stconn or the stream, helper functions are used to get, set or
reset these timers. This simplify the timers manipulations.
Read and write timeouts concerns the I/O. Thus, it is logical to move it into
the stconn. At the end, the stream is responsible to detect the timeouts. So
it is logcial to have these values in the stconn and not in the SE
descriptor. But it may change depending on the recfactoring.
So, now:
* scf->rto is used instead of req->rto
* scf->wto is used instead of res->wto
* scb->rto is used instead of res->rto
* scb->wto is used instead of req->wto
This patch removes CF_READ_ERROR and CF_WRITE_ERROR flags. We now rely on
SE_FL_ERR_PENDING and SE_FL_ERROR flags. SE_FL_ERR_PENDING is used for write
errors and SE_FL_ERROR for read or unrecoverable errors.
When a connection error is reported, SE_FL_ERROR and SE_FL_EOS are now set and a
read event and a write event are reported to be sure the stream will properly
process the error. At the stream-connector level, it is similar. When an error
is reported during a send, a write event is triggered. On the read side, nothing
more is performed because an error at this stage is enough to wake the stream
up.
A major change is brought with this patch. We stop to check flags of the
ooposite channel to report abort or timeout. It also means when an read or
write error is reported on a side, we no longer update the other side. Thus
a read error on the server side does no long lead to a write error on the
client side. This should ease errors report.
This flag was introduced in 1.3 to fix a design issue. It was untouch since
then but there is no reason to still have this trick. Note it could be good
to review what happens in HTTP with the server is waiting for the end of the
request. It could be good to be sure a client timeout is always reported.
At the stream level, the read expiration date is unset if a shutr was
received but not if the end of input was reached. If we know no more data
are excpected, there is no reason to let the read expiration date armed,
except to respect clientfin/serverfin timeout on some circumstances.
This patch could slowly be backported as far as 2.2.
It is not really a bug because it does not fix any known issue. And it is
flagged as MEDIUM because it is sensitive. But if there are some extra calls
to process_stream(), it can be an issue because, in si_update_rx(), we may
disable reading for the SC when outgoing data are blocked in the input
channel. But it is not really the process_stream() job to take care of
that. This may block data receipt.
It is an old code, mainly here to avoid wakeup in loop on the stats
applet. Today, it seems useless and can lead to bugs. An endpoint is
responsible to block the SC if it waits for some room and the opposite
endpoint is responsible to unblock it when some data are sent. The stream
should not interfere on this part.
This patch could be backported to 2.7 after a period of observation. And it
should only be backported to lower versions if an issue is reported.
With a connection, when data are received, if these data are sent to the
opposite side because the fast-forwarding is possible, the stream may be
woken up on some conditions (at the end of sc_app_chk_snd_conn()):
* The channel is shut for write
* The SC is not in the "established" state
* The stream must explicitly be woken up on write and all data was sent
* The connection was just established.
A bug on the last condition was introduced with the commit d89884153
("MEDIUM: channel: Use CF_WRITE_EVENT instead of CF_WRITE_PARTIAL"). The
stream is now woken up on any write events.
This patch fixes this issue and restores the original behavior. No backport
is needed.
The commit 7f59d68fe ("BUG/MEDIIM: stconn: Flush output data before
forwarding close to write side") introduced a regression. When the read side
is closed, the close is not forwarded to the write side if there are some
pending outgoind data. The idea is to foward data first and the close the
write side. However, when fast-forwarding is enabled and last data block is
received with the read0, the close is never forwarded.
We cannot revert the commit above because it really fix an issue. However,
we can schedule the shutdown for write by setting CF_SHUTW_NOW flag on the
write side. Indeed, it is the purpose of this flag.
To not replicate ugly and hardly maintainable code block at different places
in stconn.c, an helper function is used. Thus, sc_cond_forward_shutw() must
be called to know if the close can be fowarded or not. It returns 1 if it is
possible. In this case, the caller is responsible to forward the close to
the write side. Otherwise, if the close cannot be forwarded, 0 is
returned. It happens when it should not be performed at all. Or when it
should only be delayed, waiting for the input channel to be flushed. In this
last case, the CF_SHUTW_NOW flag is set in the output channel.
This patch should fix the issue #2033. It must be backported with the commit
above, thus at least as far as 2.2.
In applets, we stop processing when a write error (CF_WRITE_ERROR) or a shutdown
for writes (CF_SHUTW) is detected. However, any write error leads to an
immediate shutdown for writes. Thus, it is enough to only test if CF_SHUTW is
set.
Thanks to previous changes, CF_WRITE_ACTIVITY flags can be removed.
Everywhere it was used, its value is now directly used
(CF_WRITE_EVENT|CF_WRITE_ERROR).
Thanks to previous changes, CF_READ_ACTIVITY flags can be removed.
Everywhere it was used, its value is now directly used
(CF_READ_EVENT|CF_READ_ERROR).
Just like CF_READ_PARTIAL, CF_WRITE_PARTIAL is now merged with
CF_WRITE_EVENT. There a subtlety in sc_notify(). The "connect" event
(formely CF_WRITE_NULL) is now detected with
(CF_WRITE_EVENT + sc->state < SC_ST_EST).
CF_READ_PARTIAL flag is now merged with CF_READ_EVENT. It means
CF_READ_EVENT is set when a read0 is received (formely CF_READ_NULL) or when
data are received (formely CF_READ_ACTIVITY).
There is nothing special here, except conditions to wake the stream up in
sc_notify(). Indeed, the test was a bit changed to reflect recent
change. read0 event is now formalized by (CF_READ_EVENT + CF_SHUTR).
As for CF_READ_NULL, it appears CF_WRITE_NULL and other write events on a
channel are mainly used to wake up the stream and may be replace by on write
event.
In this patch, we introduce CF_WRITE_EVENT flag as a replacement to
CF_WRITE_EVENT_NULL. There is no breaking change for now, it is just a
rename. Gradually, other write events will be merged with this one.
CF_READ_NULL flag is not really useful and used. It is a transient event
used to wakeup the stream. As we will see, all read events on a channel may
be resumed to only one and are all used to wake up the stream.
In this patch, we introduce CF_READ_EVENT flag as a replacement to
CF_READ_NULL. There is no breaking change for now, it is just a
rename. Gradually, other read events will be merged with this one.
In process_stream(), we wait to have an empty output channel to forward a
close to the write side (a shutw). However, at the stream-connector level,
when a close is detected on one side and we don't want to keep half-close
connections, the shutw is unconditionally forwarded to the write side. This
typically happens on server side.
At first glance, this bug may truncate messages. But depending on the muxes
and the stream states, the bug may be more visible. On recent versions
(2.8-dev and 2.7) and on 2.2 and 2.0, the stream may be freezed, waiting for
the client timeout, if the client mux is unable to forward data because the
client is too slow _AND_ the response channel is not empty _AND_ the server
closes its connection _AND_ the server mux has forwarded all data to the
upper layer _AND_ the client decides to send some data and to close its
connection. On 2.6 and 2.4, it is worst. Instead of a freeze, the client mux
is woken up in loop.
Of course, conditions are pretty hard to meet. Especially because it is highly
time dependent. For what it's worth, I reproduce it with tcploop on client and
server sides and a basic HTTP configuration for HAProxy:
* client: tcploop -v 8889 C S:"GET / HTTP/1.1\r\nConnection: upgrade\r\n\r\n" P5000 S:"1234567890" K
* server: tcploop -v 8000 L A R S:"HTTP/1.1 101 ok\r\nConnection: upgrade\r\n\r\n" P2000 S2660000 F R
On 2.8-dev, without this patch, the stream is freezed and when the client
connection timed out, client data are truncated and '--cL' is reported in
logs. With the patch, the client data are forwarded to the server and the
connection is closed. A '--CD' is reported in logs.
It is an old bug. It was probably introduced with the multiplexers. To fix
it, in stconn (Formerly the stream-interface), we must wait all output data
be flushed before forwarding close to write side.
This patch must be backported as far as 2.2 and must be evaluated for 2.0.
SE_FL_ERR_PENDING flag is used when there is still data to be read. So we
must take care to not set SE_FL_ERROR too early. Thus, on sending path, it
must be set if SE_FL_EOS was already set.
If stream_new() fails after the frontend SC is attached, the underlying SE
descriptor is not properly reset. Among other things, SE_FL_ORPHAN flag is
not set again. Because of this error, a BUG_ON() is triggered when the mux
stream on the frontend side is destroyed.
Thus, now, when stream_new() fails, SE_FL_ORPHAN flag is set on the SE
descriptor and its stream-connector is set to NULL.
This patch should solve the issue #1880. It must be backported to 2.6.
gcc-6.x and 7.x emit build warnings about sc possibly being null upon
return from sc_detach_endp(). This actually is not the case and the
compiler is a little bit overzealous there, but there exists code
paths that can make this analysis non-trivial so let's at least add
a similar BUG_ON() to let both the compiler and the deverloper know
this doesn't happen.
This should be backported to 2.6.
Since commit ae024ced0 ("MEDIUM: stream-int/stream: Use connect expiration
instead of SI expiration"), the connect expiration date is per-stream. So
there is only one expiration date instead of one per side, front and
back. So when a stream-connector is processed, we must test if it is a
frontend or a backend stconn before updating the connect expiration
date. Indeed, the frontend stconn must not reset the connect expiration
date.
This bug may have several side effect. One known bug is about peer sessions
blocked because the frontend peer applet is in ST_CLO state and its backend
connection is in ST_TAR state but without connect expiration date.
This patch should fix the issue #1791 and #1792. It must be backported to
2.6.
When the protocol is changed for a client connection at the stream level
(from TCP to H1/H2), there are two cases. The stream may be reused or
not. The first case, when the stream is reused is working. The second one is
buggy since the conn-stream refactoring and leads to a crash.
In this case, the new mux don't reuse the stream. It must be silently
aborted. However, its front stream connector is still referencing the
connection. So it must be detached. But it must be performed in two stages,
to be sure to not loose the context for the upgrade and to be able to
rollback on error. So now, before the upgrade, we prepare to detach the
stconn and it is finally detached if the upgrade succeeds. There is a trick
here. Because we pretend the stconn is detached but its state is preserved.
This patch must be backported to 2.6.
in .chk_snd applet callback function, we must not wake up an applet if
SE_FL_WONT_CONSUME flag is set. Indeed, if an applet explicitly specify it
will not consume any outgoing data, it is useless to wake it up when more
data are sent. Note the applet may still be woken up for another reason. In
this case SE_FL_WONT_CONSUME flag will be removed. It is the applet
responsibility to set it again, if necessary.
This patch must be backported to 2.6 after an observation period. On earlier
versions, the bug probably exists too. However, because a massive
refactoring was performed in 2.6, the patch will have to be adapted and
carefully reviewed/tested if it is backported..
The only two places where it was used was to carefully preserve the
SE_FL_WILL_CONSUME flag (since others are irrelevant there and the
previous RXBLK* flags moved to the stconn). Now that the flag is
cleared by default there's no need to re-created a fresh new one
when replacing the descriptor, so we can eliminate that remaining
trick.
Function arguments and local variables called "cs" were renamed to "sc"
to avoid future confusion. The change is huge (~580 lines), so extreme
care was given not to change anything else.
There's no more reason for keepin the code and definitions in conn_stream,
let's move all that to stconn. The alphabetical ordering of include files
was adjusted.
