MAJOR: fd: grab the tgid before manipulating running
We now grab a reference to the FD's tgid before manipulating the
running_mask so that we're certain it corresponds to our own group
(hence bits), and we drop it once we've set the bit. For now there's
no measurable performance impact in doing this, which is great. The
lock can be observed by perf top as taking a small share of the time
spent in fd_update_events(), itself taking no more than 0.28% of CPU
under 8 threads.
However due to the fact that the thread groups are not yet properly
spread across the pollers and the thread masks are still wrong, this
will trigger some BUG_ON() in fd_insert() after a few tens of thousands
of connections when threads other than those of group 1 are reached,
and this is expected.
MINOR: fd: add fd_get_running() to atomically return the running mask
The running mask is only valid if the tgid is the expected one. This
function takes a reference on the tgid before reading the running mask,
so that both are checked at once. It returns either the mask or zero if
the tgid differs, thus providing a simple way for a caller to check if
it still holds the FD.
MINOR: fd: add functions to manipulate the FD's tgid
The FD's tgid is refcounted and must be atomically manipulated. Function
fd_grab_tgid() will increase the refcount but only if the tgid matches the
one in argument (likely the current one). fd_claim_tgid() will be used to
self-assign the tgid after waiting for its refcount to reach zero.
fd_drop_tgid() will be used to drop a temporarily held tgid. All of these
are needed to prevent an FD from being reassigned to another group, either
when inspecting/modifying the running_mask, or when checking for updates,
in order to be certain that the mask being seen corresponds to the desired
group. Note that once at least one bit is set in the running mask of an
active FD, it cannot be closed, thus not migrated, thus the reference does
not need to be held long.
MEDIUM: fd: add the tgid to the fd and pass it to fd_insert()
The file descriptors will need to know the thread group ID in addition
to the mask. This extends fd_insert() to take the tgid, and will store
it into the FD.
In the FD, the tgid is stored as a combination of tgid on the lower 16
bits and a refcount on the higher 16 bits. This allows to know when it's
really possible to trust the tgid and the running mask. If a refcount is
higher than 1 it indeed indicates another thread else might be in the
process of updating these values.
Since a closed FD must necessarily have a zero refcount, a test was
added to fd_insert() to make sure that it is the case.
MINOR: fd: make fd_insert() apply the thread mask itself
It's a bit ugly to see that half of the callers of fd_insert() have to
apply all_threads_mask themselves to the bit field they're passing,
because usually it comes from a listener that may have other bits set.
Let's make the function apply the mask itself.
After a poller's ->clo() was called to completely terminate operations
on an FD, there's no reason for keeping updates on this FD, so if any
updates were already programmed it would be nice if we could delete them.
Tests show that __fd_clo() is called roughly half of the time with the
last FD from the local update list, which possibly makes sense if a close
has to appear after a polling change resulting from an incomplete read or
the end of a send().
We can detect this and remove the last entry, which gives less work to
do during the update() call, and eliminates most of the poll_drop_fd
event reports.
Note that while tempting, this must not be backported because it's only
safe to be done now that fd_delete_orphan() clears the update mask as
we need to be certain not to miss it:
- if the update mask is kept up with no entry, we can miss future
updates ;
- if the update mask is cleared too fast, it may result in failure
to add a shared event.
The update-list needs to be per-group because its inspection is based
on a mask and we need to be certain when scanning it if a mask is for
the same thread or another one. Once per-group there's no doubt about
it, even if the FD's polling changes, the entry remains valid. It will
be needed to check the tgid though.
Note that a soft-stop or pause/resume might not necessarily work here
with tgroups>1, because the operation might be delivered to a thread
that doesn't belong to the group and whoe update mask will not reflect
one that is interesting here. We can't do better at this stage.
Dealing with long-lasting updates that outlive a close() is always
going to be quite a problem, not because of the thread that will
discover such updates late, but mostly due to the shared update_list
that will have an entry on hold making it difficult to reuse it, and
requiring that the fd's tgid is changed and the update_mask reset
from a safe location.
After careful inspection, it turns out that all our pollers that support
automatic event removal upon close() do not need any extra bookkeeping,
and that poll and select that use an internal representation already
provide a poller->clo() callback that is already used to update the
local event. As such, it is already safe to reset the update mask and
to remove the event from the shared list just before the final close,
because nothing remains to be done with this FD by the poller.
Doing so considerably simplifies the handling of updates, which will
only have to be inspected by the pollers, while the writers can continue
to consider that the entries are always valid. Another benefit is that
it will be possible to reduce contention on the update_list by just
having one update_list per group (left to be done later if needed).
MEDIUM: conn: make conn_backend_get always scan the same group
We don't want to pick idle connections from another thread group,
this would be very slow by forcing to share undesirable data.
This patch makes sure that we start seeking from the current thread
group's threads only and loops over that range exclusively.
It's worth noting that the next_takeover pointer remains per-server
and will bounce when multiple groups use it at the same time. But we
preserve the perturbation by applying a modulo when retrieving it,
so that when groups are of the same size (most common case), the
index will not even change. At this time it doesn't seem worth
storing one index per group in servers, but that might be an option
if any contention is detected later.
MINOR: task: move the niced_tasks counter to the thread group context
This one is only used as a hint to improve scheduling latency, so there
is no more point in keeping it global since each thread group handles
its own run q
MEDIUM: task/thread: move the task shared wait queues per thread group
Their migration was postponed for convenience only but now's time for
having the shared wait queues per thread group and not just per process,
otherwise the WQ lock uses a huge amount of CPU alone.
Since commit d2494e048 ("BUG/MEDIUM: peers/config: properly set the
thread mask") there must not remain any single case of a receiver that
is bound nowhere, so there's no need anymore for thread_mask().
We're adding a test in fd_insert() to make sure this doesn't happen by
accident though, but the function was removed and its rare uses were
replaced with the original value of the bind_thread msak.
MINOR: cli/threads: always bind CLI to thread group 1
When using multiple groups, the stats socket starts to emit errors and
it's not natural to have to touch the global section just to specify
"thread 1/all". Let's pre-attach these sockets to thread group 1. This
will cause errors when trying to change the group but this really is not
a problem for now as thread groups are not enabled by default. This will
make sure configs remain portable and may possibly be relaxed later.
MINOR: mworker/threads: limit the mworker sockets to group 1
As a side effect of commit 34aae2fd1 ("MEDIUM: mworker: set the iocb of
the socketpair without using fd_insert()"), a config may now refuse to
start if there are multiple groups configured because the default bind
mask may span over multiple groups, and it is not possible to force it
to work differently.
Let's just assign thread group 1 to the master<->worker sockets so that
the thread bindings automatically resolve to a single group. The same was
done for the master side of the socket even if it's not used. It will
avoid being forgotten in the future.
MEDIUM: cpu-map: replace the process number with the thread group number
The principle remains the same, but instead of having a single process
and ignoring extra ones, now we set the affinity masks for the respective
threads of all groups.
MEDIUM: poller: disable thread-groups for poll() and select()
These old legacy pollers are not designed for this. They're still
using a shared list of events for all threads, this will not scale at
all, so there's no point in enabling thread-groups there. Modern
systems have epoll, kqueue or event ports and do not need these ones.
We arrange for failing at boot time, only when thread-groups > 1 so
that existing setups will remain unaffected.
If there's a compelling reason for supporting thread groups with these
pollers in the future, the rework should not be too hard, it would just
consume a lot of memory to have an fd_evts[] array per thread, but that
is doable.
MEDIUM: poller: program the update in fd_update_events() for a migrated FD
When an FD is migrated, all pollers program an update. That's useless
code duplication, and when thread groups will be supported, this will
require an extra round of locking just to verify the update_mask on
return. Let's just program the update direction from fd_update_events()
as it already does for closed FDs, this becomes more logical.
MEDIUM: proto: stop protocols under thread isolation during soft stop
protocol_stop_now() is called from do_soft_stop_now() running on any
thread that received the signal. The problem is that it will call some
listener handlers to close the FD, resulting in an fd_delete() being
called from the wrong group. That's not clean and we cannot even rely
on the thread mask to show up.
One interesting long-term approach could be to have kill queues for
FDs, and maybe we'll need them in the long run. However that doesn't
work well for listeners in this situation.
Let's simply isolate ourselves during this instant. We know we'll be
alone dealing with the close and that the FD will be instantly deleted
since not in use by any other thread. It's not the cleanest solution
but it should last long enough without causing trouble.
MEDIUM: debug/threads: make the lock debugging take tgroups into account
Since we have to use masks to verify owners/waiters, we have no other
option but to have them per group. This definitely inflates the size
of the locks, but this is only used for extreme debugging anyway so
that's not dramatic.
Thus as of now, all masks in the lock stats are local bit masks, derived
from ti->ltid_bit. Since at boot ltid_bit might not be set, we just take
care of this situation (since some structs are initialized under look
during boot), and use bit 0 from group 0 only.
CLEANUP: fd: get rid of the __GET_{NEXT,PREV} macros
They were initially made to deal with both the cache and the update list
but there's no cache anymore and keeping them for the update list adds a
lot of obfuscation that is really not desired. Let's get rid of them now.
Their purpose was simply to get a pointer to fdtab[fd].update.{,next,prev}
in order to perform atomic tests and modifications. The offset passed in
argument to the functions (fd_add_to_fd_list() and fd_rm_from_fd_list())
was the offset of the ->update field in fdtab, and as it's not used anymore
it was removed. This also removes a number of casts, though those used by
the atomic ops have to remain since only scalars are supported.
MEDIUM: config: remove the "process" keyword on "bind" lines
It was deprecated, marked for removal in 2.7 and was already emitting a
warning, let's get rid of it. Note that we've kept the keyword detection
to suggest to use "thread" instead.
MEDIUM: config: remove deprecated "bind-process" directives from frontends
This was already causing a deprecation warning and was marked for removal
in 2.7, now it happens. An error message indicates this doesn't exist
anymore.
CLEANUP: applet: remove the obsolete command context from the appctx
The "ctx" and "st2" parts in the appctx were marked for removal in 2.7
and were emulated using memcpy/memset etc for possible external code.
Let's remove this now.
MINOR: cli/activity: add a thread number argument to "show activity"
The output of "show activity" can be so large that the output is visually
unreadable on a screen. Let's add an option to filter on the desired
column (actually the thread number), use "0" to report only the first
column (aggregated/sum/avg), and use "-1", the default, for the normal
detailed dump.
DEBUG: cli: add a new "debug dev deadlock" expert command
This command will create the requested number of tasks competing on a
lock, resulting in triggering the watchdog and crashing the process.
This will help stress the watchdog and inspect the lock debugging parts.
BUG/MEDIUM: debug: fix parallel thread dumps again
The previous attempt to fix thread dumps in commit 672972604 ("BUG/MEDIUM:
debug: fix possible hang when multiple threads dump at once") still had
some shortcomings. Sometimes parallel dumps are jerky essentially due to
the way that threads synchronize on startup and end. In addition the risk
of waiting forever for a stopped thread exists, and panics happening in
parallel to thread dumps are not more reliable either.
This commit revisits the state transitions so that all threads may request
a dump in parallel, that all of them wait for each other in the handler,
and that one thread is responsible for counting every other and checking
that the total matches the number of active threads.
Then for stopping there's a finishing phase that all threads wait for so
that none quits this area too early. Given that we now know the number of
participants to the dump, we can let them each decrement the counter when
leaving so that another dump may only start after the last participant
has completely left.
Now many thread dumps in parallel are running fine, so do panics. No
backport is needed as this was the result of the changes for thread
groups.
Some panic dumps are mangled or truncated due to the watchdog firing at
the same time on multiple threads and calling ha_panic() simultaneously.
What may happen in this case is that the second one waits for the first
one to finish but as soon as it's done the second one resets the buffer
and dumps again, sometimes resetting the first one's dump. Also the first
one's abort() may trigger while the second one is currently dumping,
resulting in a full dump followed by a truncated one, leading to
confusion. Sometimes some lines appear in the middle of a dump as well.
It doesn't happen often and is easier to trigger by causing massive
deadlocks.
There's no reason for the process to resist to a panic, so we can safely
add a counter and no nothing on subsequent calls. Ideally we'd wait there
forever but as this may happen inside a signal handler (e.g. watchdog),
it doesn't always work, so the easiest thing to do is to return so that
the thread is interrupted as soon as possible and brought to the debug
handler to be dumped.
This should be backported, at least to 2.6 and possibly to older versions
as well.
BUG/MINOR: thread: use the correct thread's group in ha_tkillall()
In ha_tkillall(), the current thread's group was used to check for the
thread being running instead of using the target thread's group mask.
Most of the time it would not have any effect unless some groups are
uneven where it can lead to incomplete thread dumps for example.
BUG/MEDIUM: cli/threads: make "show threads" more robust on applets
Running several concurrent "show threads" in loops might occasionally
cause a segfault when trying to retrieve the stream from appctx_sc()
which may be null while the applet is finishing. It's not easy to
reproduce, it requires 3-5 sessions in parallel for about a minute
or so. The appctx_sc must be checked before passing it to sc_strm().
This must be backported to 2.6 which also has the bug.
BUG/MINOR: threads: produce correct global mask for tgroup > 1
In thread_resolve_group_mask(), if a global thread number is passed
and it belongs to a group greater than 1, an incorrect shift resulted
in shifting that ID again which made it appear nowhere or in a wrong
group possibly. The bug was introduced in 2.5 with commit 627def9e5
("MINOR: threads: add a new function to resolve config groups and
masks") though the groups only starts to be usable in 2.7, so there
is no impact for this bug, hence no backport is needed.
Amaury Denoyelle [Mon, 28 Mar 2022 12:53:45 +0000 (14:53 +0200)]
MINOR: h3: implement graceful shutdown with GOAWAY
Implement graceful shutdown as specified in RFC 9114. A GOAWAY frame is
generated with stream ID to indicate range of processed requests.
This process is done via the release app protocol operation. The MUX
is responsible to emit the generated GOAWAY frame after app release. A
CONNECTION_CLOSE will be emitted once there is no unacknowledged STREAM
frames.
Amaury Denoyelle [Mon, 13 Jun 2022 15:09:01 +0000 (17:09 +0200)]
MINOR: mux-quic: send one last time before release
Call qc_send() on qc_release(). This is mostly useful for application
protocol with a connection closing procedure. Most notably, this will be
useful to implement HTTP/3 GOAWAY emission.
This change is purely cosmetic. qc_release() function is moved just
before qc_io_cb(). It's cleaner as it brings it closer where it is used.
More importantly, this will be required to be able to use it in
qc_send() function.
MEDIUM: quic: send CONNECTION_CLOSE on released MUX
Send a CONNECTION_CLOSE if the MUX has been released and all STREAM data
are acknowledged. This is useful to prevent a client from trying to use
a connection which have the upper layer closed.
To implement this a new function qc_check_close_on_released_mux() has
been added. It is called on QUIC MUX release notification and each time
a qc_stream_desc has been released.
This commit is associated with the previous one :
MINOR: mux-quic/h3: schedule CONNECTION_CLOSE on app release
Both patches are required to prevent the risk of browsers stuck on
webpage loading if MUX has been released. On CONNECTION_CLOSE reception,
the client will reopen a new QUIC connection.
MINOR: mux-quic/h3: prepare CONNECTION_CLOSE on release
When MUX is released, a CONNECTION_CLOSE frame should be emitted. This
will ensure that the client does not use anymore a half-dead connection.
App protocol layer is responsible to provide the error code via release
callback. For HTTP/3 NO_ERROR is used as specified in RFC 9114. If no
release callback is provided, generic QUIC NO_ERROR code is used. Note
that a graceful shutdown is used : quic_conn must emit CONNECTION_CLOSE
frame when possible. This will be provided in another patch.
This change should limit the risk of browsers stuck on webpage loading
if MUX has been released. On CONNECTION_CLOSE reception, the client will
reopen a new QUIC connection.
Adjust qcc_emit_cc_app() to allow the delay of emission of a
CONNECTION_CLOSE. This will only set the error code but the quic-conn
layer is not flagged for immediate close. The quic-conn will be
responsible to shut the connection when deemed suitable.
This change will allow to implement application graceful shutdown, such
as HTTP/3 with GOAWAY emission. This will allow to emit closing frames
on MUX release. Once all work is done at the lower layer, the quic-conn
should emit a CONNECTION_CLOSE with the registered error code.
Define a new structure quic_err to abstract a QUIC error type. This
allows to easily differentiate a transport and an application error
code. This simplifies error transmission from QUIC MUX and H3 layers.
This new type is defined in quic_frame module. It is used to replace
<err_code> field in <quic_conn>. QUIC_FL_CONN_APP_ALERT flag is removed
as it is now useless.
Utility functions are defined to be able to quickly instantiate
transport, tls and application errors.
quic_frame-t.h and xprt_quic-t.h include themselves mutually. This may
cause some troubles later.
In fact, xprt_quic does not need to include quic_frame so remove this.
And as quic_frame is a generic source file which is included in multiple
places, it is useful to also remove the xprt_quic include in it. Use
forward declaration for this.
BUG/MEDIUM: debug: fix possible hang when multiple threads dump at once
A bug in the thread dumper was introduced by commit 00c27b50c ("MEDIUM:
debug: make the thread dumper not rely on a thread mask anymore"). If
two or more threads try to trigger a thread dump exactly at the same
time, the second one may loop indefinitely trying to set the value to 1
while the other ones will wait for it to finish dumping before leaving.
This is a consequence of a logic change using thread numbers instead of
a thread mask, as threads do not need to see all other ones there anymore.
Implement support for STOP_SENDING frame parsing. The stream is resetted
as specified by RFC 9000. This will automatically interrupt all future
send operation in qc_send(). A RESET_STREAM will be sent with the code
extracted from the original STOP_SENDING frame.
Implement functions to be able to reset a stream via RESET_STREAM.
If needed, a qcs instance is flagged with QC_SF_TO_RESET to schedule a
stream reset. This will interrupt all future send operations.
On stream emission, if a stream is flagged with QC_SF_TO_RESET, a
RESET_STREAM frame is generated and emitted to the transport layer. If
this operation succeeds, the stream is locally closed. If upper layer is
instantiated, error flag is set on it.
MINOR: mux-quic: use stream states to mark as detached
Adjust condition to detach a qcs instance : if the stream is not locally
close it is not directly free. This should improve stream closing by
ensuring that either FIN or a RESET_STREAM is sent before destroying it.
Implement a basic state machine to represent stream lifecycle. By
default a stream is idle. It is marked as open when sending or receiving
the first data on a stream.
Bidirectional streams has two states to represent the closing on both
receive and send channels. This distinction does not exists for
unidirectional streams which passed automatically from open to close
state.
This patch is mostly internal and has a limited visible impact. Some
behaviors are slightly updated :
* closed streams are garbage collected at the start of io handler
* send operation is interrupted if a stream is close locally
Outside of this, there is no functional change. However, some additional
BUG_ON guards are implemented to ensure that we do not conduct invalid
operation on a stream. This should strengthen the code safety. Also,
stream states are displayed on trace which should help debugging.
MINOR: mux-quic: support stream opening via MAX_STREAM_DATA
MAX_STREAM_DATA can be used as the first frame of a stream. In this
case, the stream should be opened, if it respects flow-control limit. To
implement this, simply replace plain lookup in stream tree by
qcc_get_qcs() at the start of the parsing function. This automatically
takes care of opening the stream if not already done.
As specified by RFC 9000, if MAX_STREAM_DATA is receive for a
receive-only stream, a STREAM_STATE_ERROR connection error is emitted.
MINOR: mux-quic: do not ack STREAM frames on unrecoverable error
Improve return path for qcc_recv() on STREAM parsing. It returns 0 on
success. On error, a non-zero value is returned which indicates to the
caller that the packet containing the frame should not be acknowledged.
When qcc_recv() generates a CONNECTION_CLOSE or RESET_STREAM, either
directly or via qcc_get_qcs(), an error is returned which ensure that no
acknowledgement is generated. This required an adjustment on
qcc_get_qcs() API which now returns a success/error code. The stream
instance is returned via a new out argument.
MINOR: mux-quic: filter send/receive-only streams on frame parsing
Extend the function qcc_get_qcs() to be able to filter send/receive-only
unidirectional streams. A connection error STREAM_STATE_ERROR is emitted
if this new filter does not match.
This will be useful when various frames handlers are converted with
qcc_get_qcs(). Depending on the frame type, it will be easy to filter on
the forbidden stream types as specified in RFC 9000.
Implement a simple function to notify a possible subscriber or wake up
the upper layer if a special condition happens on a stream. For the
moment, this is only used to replace identical code in
qc_wake_some_streams().
Rename qc_release_detached_streams() to qc_purge_streams(). The aim is
to have a more generic name. It's expected to complete this function to
add other criteria to purge dead streams.
Also the function documentation has been corrected. It does not return a
number of streams. Instead it is a boolean value, to true if at least
one stream was released.
REORG: mux-quic: rename stream initialization function
Rename both qcc_open_stream_local/remote() functions to
qcc_init_stream_local/remote(). This change is purely cosmetic. It will
reduces the ambiguity with the soon to be implemented OPEN states for
QCS instances.
BUG/MEDIUM: mux-quic: fix server chunked encoding response
QUIC MUX was not able to correctly deal with server response using
chunked transfer-encoding. All data will be transfered correctly to the
client but the FIN bit is missing. The transfer will never stop as the
client will wait indefinitely for the FIN bit.
This bug happened because the HTX message representing a chunked encoded
payload contains a final empty block with the EOM flag. However,
emission is skipped by QUIC MUX if there is no data to transfer. To fix
this, the condition was completed to ensure that there is no need to
send the FIN signal. If this is false, data emission will proceed even
if there is no data : this will generate an empty QUIC STREAM frame with
FIN set which will mark the end of the transfer.
To ensure that a FIN STREAM frame is sent only one time,
QC_SF_FIN_STREAM is resetted on send confirmation from the transport in
qcc_streams_sent_done().
This bug was reproduced when dealing with chunked transfer-encoding
response for the HTTP server.
BUILD: http: silence an uninitialized warning affecting gcc-5
When building with gcc-5, one can see this warning:
src/http_fetch.c: In function 'smp_fetch_meth':
src/http_fetch.c:356:6: warning: 'htx' may be used uninitialized in this function [-Wmaybe-uninitialized]
sl = http_get_stline(htx);
^
It's wrong since the only way to reach this code is to have met the
same condition a few lines before and initialized the htx variable.
The reason in fact is that the same test happens on different variables
of distinct types, so the compiler possibly doesn't know that the
condition is the same. Newer gcc versions do not have this problem.
Let's just move the assignment earlier and have the exact same test,
as it's sufficient to shut this up. This may have to be backported
to 2.6 since the code is the same there.
In 2.6, 8a0fd3a36 ("BUILD: debug: work around gcc-12 excessive
-Warray-bounds warnings") disabled some warnings that were reported
around the the BUG() statement. But the -Wnull-dereference warning
isn't known from gcc-5, it only arrived in gcc-6, hence makes gcc-5
complain loudly that it doesn't know this directive. Let's just
condition this one to gcc-6.
Between 1.8 and 1.9 commit d9e7e36c6 ("BUG/MEDIUM: epoll/threads: use one
epoll_fd per thread") split the epoll poller to use one poller per thread
(and this was backported to 1.8). This patch added a call to epoll_ctl(DEL)
on return from the I/O handler as a safe way to deal with a detected thread
migration when that code was still quite fragile. One aspect of this choice
was that by then we wanted to maintain support for the rare old bogus epoll
implementations that failed to remove events on close(), so risking to lose
the event was not an option.
Later in 2.5, commit 200bd50b7 ("MEDIUM: fd: rely more on fd_update_events()
to detect changes") changed the code to perform most of the operations
inside fd_update_events(), but it maintained that oddity, to the point that
strictly all pollers except epoll now just add an update to be dealt with at
the next round.
This approach is much more efficient, because under load and server-side
connection reuse, it's perfectly possible for a thread to see the same FD
several times in a poll loop, the first time to relinquish it after a
migration, then the other thread makes a request, gets its response, and
still during the same loop for the first one, grabbing an idle connection
to send a request and wait for a response will program a new update on
this FD. By using a synchronous epoll_ctl(DEL), we effectively lose the
opportunity to aggregate certain changes in the same update.
Some tests performed locally with 8 threads and one server show that on
average, by using an update instead of a synchronous call, we reduce the
number of epoll_ctl() calls by 25-30% (under low loads it will probably
not change anything).
So this patch implements the same method for all pollers and replaces the
synchronous epoll_ctl() with an update.
BUG/MEDIUM: mux-h1: Handle connection error after a synchronous send
Since commit d1480cc8 ("BUG/MEDIUM: stream-int: do not rely on the
connection error once established"), connection errors are not handled
anymore by the stream-connector once established. But it is a problem for
the H1 mux when an error occurred during a synchronous send in
h1_snd_buf(). Because in this case, the connction error is just missed. It
leads to a session leak until a timeout is reached (client or server).
To fix the bug, the connection flags are now checked in h1_snd_buf(). If
there is an error, it is reported to the stconn level by setting SF_FL_ERROR
flags. But only if there is no pending data in the input buffer.
This patch should solve the issue #1774. It must be backported as far as
2.2.
BUG/MEDIUM: http-ana: Don't wait to have an empty buf to switch in TUNNEL state
When we want to establish a tunnel on a side, we wait to have flush all data
from the buffer. At this stage the other side is at least in DONE state. But
there is no real reason to wait. We are already in DONE state on its
side. So all the HTTP message was already forwarded or planned to be
forwarded. Depending on the scheduling if the mux already started to
transfer tunneled data, these data may block the switch in TUNNEL state and
thus block these data infinitly.
This bug exists since the early days of HTX. May it was mandatory but today
it seems useless. But I honestly don't remember why this prerequisite was
added. So be careful during the backports.
This patch should be backported with caution. At least as far as 2.4. For
2.2 and 2.0, it seems to be mandatory too. But a review must be performed.
BUG/MINOR: mux-h1: Be sure to commit htx changes in the demux buffer
When a buffer area is casted to an htx message, depending on the method
used, the underlying buffer may be updated or not. The htxbuf() function
does not change the buffer state. We assume the buffer was already prepared
to store an htx message. htx_from_buf() on its side, updates the
buffer. With the first function, we only need to commit changes to the
underlying buffer if the htx message is changed. With last one, we must
always commit the changes. The idea is to be sure to keep non-empty HTX
messages while an empty message must be lead to an empty buffer after
commit.
All that said because in h1_process_demux(), the changes is not always
committed as expected. When the demux is blocked, we just leave the
function. So it is possible to have an empty htx message stored in a buffer
that appears non-empty. It is not critical, but the buffer cannot be
released in this state. And we should always release an empty buffer.
MEDIUM: mworker/systemd: send STATUS over sd_notify
The sd_notify API is not able to change the "Active:" line in "systemcl
status". However a message can still be displayed on a "Status: " line,
even if the service is still green and "active (running)".
When startup succeed the Status will be set to "Ready.", upon a reload
it will be set to "Reloading Configuration." If the configuration
succeed "Ready." again. However if the reload failed, it will be set to
"Reload failed!".
Keep in mind that the "Active:" line won't change upon a reload failure,
and will still be green.
BUG/MEDIUM: http-fetch: Don't fetch the method if there is no stream
The "method" sample fetch does not perform any check on the stream existence
before using it. However, for errors triggered at the mux level, there is no
stream. When the log message is formatted, this sample fetch must fail. It
must also fail when it is called from a health-check.
BUG/MEDIUM: h1: Improve authority validation for CONNCET request
From time to time, users complain to get 400-Bad-request responses for
totally valid CONNECT requests. After analysis, it is due to the H1 parser
performs an exact match between the authority and the host header value. For
non-CONNECT requests, it is valid. But for CONNECT requests the authority
must contain a port while it is often omitted from the host header value
(for default ports).
So, to be sure to not reject valid CONNECT requests, a basic authority
validation is now performed during the message parsing. In addition, the
host header value is normalized. It means the default port is removed if
possible.
This patch should solve the issue #1761. It must be backported to 2.6 and
probably as far as 2.4.
http_is_default_port() can be used to test if a port is a default HTTP/HTTPS
port. A scheme may be specified. In this case, it is used to detect defaults
ports, 80 for "http://" and 443 for "https://". Otherwise, with no scheme, both
are considered as default ports.
MINOR: http: Add function to get port part of a host
http_get_host_port() function can be used to get the port part of a host. It
will be used to get the port of an uri authority or a host header
value. This function only look for a port starting from the end of the
host. It is the caller responsibility to call it with a valid host value. An
indirect string is returned.
BUG/MINOR: http-htx: Fix scheme based normalization for URIs wih userinfo
The scheme based normalization is not properly handled the URI's userinfo,
if any. First, the authority parser is not called with "no_userinfo"
parameter set. Then it is skipped from the URI normalization.
BUG/MINOR: peers: fix possible NULL dereferences at config parsing
Patch 49f6f4b ("BUG/MEDIUM: peers: fix segfault using multiple bind on
peers sections") introduced possible NULL dereferences when parsing the
peers configuration.
Fix the issue by checking the return value of bind_conf_uniq_alloc().
CLEANUP: thread: also remove a thread's bit from stopping_threads on stop
As much as possible we should take care of not leaving bits from stopped
threads in shared thread masks. It can avoid issues like the previous
fix and will also make debugging less confusing.
BUG/MEDIUM: thread: mask stopping_threads with threads_enabled when checking it
When soft-stopping, there's a comparison between stopping_threads and
threads_enabled to make sure all threads are stopped, but this is not
correct and is racy since the threads_enabled bit is removed when a
thread is stopped but not its stopping_threads bit. The consequence is
that depending on timing, when stopping, if the first stopping thread
is fast enough to remove its bit from threads_enabled, the other threads
will see that stopping_threads doesn't match threads_enabled anymore and
will wait forever. As such the mask must be applied to stopping_threads
during the test. This issue was introduced in recent commit ef422ced9
("MEDIUM: thread: make stopping_threads per-group and add stopping_tgroups"),
no backport is needed.
BUG/MINOR: http-act: Properly generate 103 responses when several rules are used
When several "early-hint" rules are used, we try, as far as possible, to
merge links into the same 103-early-hints response. However, it only works
if there is no ACLs. If a "early-hint" rule is not executed an invalid
response is generated. the EOH block or the start-line may be missing,
depending on the rule order.
To fix the bug, we use the transaction status code. It is unused at this
stage. Thus, it is set to 103 when a 103-early-hints response is in
progress. And it is reset when the response is forwarded. In addition, the
response is forwarded if the next rule is an "early-hint" rule with an
ACL. This way, the response is always valid.
BUG/MINOR: http-check: Preserve headers if not redefined by an implicit rule
When an explicit "http-check send" rule is used, if it is the first one, it
is merge with the implicit rule created by "option httpchk" statement. The
opposite is also true. Idea is to have only one send rule with the merged
info. It means info defined in the second rule override those defined in the
first one. However, if an element is not defined in the second rule, it must
be ignored, keeping this way info from the first rule. It works as expected
for the method, the uri and the request version. But it is not true for the
header list.
For instance, with the following statements, a x-forwarded-proto header is
added to healthcheck requests:
option httpchk
http-check send meth GET hdr x-forwarded-proto https
while by inverting the statements, no extra headers are added:
http-check send meth GET hdr x-forwarded-proto https
option httpchk
Now the old header list is overriden if the new one is not empty.
This patch should fix the issue #1772. It must be backported as far as 2.2.
CLEANUP: bwlim: Set pointers to NULL when memory is released
Calls to free() are replaced by ha_free(). And otherwise, the pointers are
explicitly set to NULL after a release. There is no issue here but it could
help debugging sessions.
BUG/MEDIUM: peers/config: properly set the thread mask
The peers didn't have their bind_conf thread mask nor group set, because
they're still not part of the global proxy list. Till 2.6 it seems it does
not have any visible impact, since most listener-oriented operations pass
through thread_mask() which detects null masks and turns them to
all_threads_mask. But starting with 2.7 it becomes a problem as won't
permit these null masks anymore.
This patch duplicates (yes, sorry) the loop that applies to the frontend's
bind_conf, though it is simplified (no sharding, etc).
As the code is right now, it simply seems impossible to trigger the second
(and largest) part of the check when leaving thread_resolve_group_mask()
on success, so it looks like it might be removed.
No backport is needed, unless a report in 2.6 or earlier mentions an issue
with a null thread_mask.
BUG/MINOR: peers/config: always fill the bind_conf's argument
Some generic frontend errors mention the bind_conf by its name as
"bind '%s'", but if this is used on peers "bind" lines it shows
"(null)" because the argument is set to NULL in the call to
bind_conf_uniq_alloc() instead of passing the argument. Fortunately
that's trivial to fix.
MINOR: mux-quic: emit FINAL_SIZE_ERROR on invalid STREAM size
Add a check on stream size when the stream is in state Size Known. In
this case, a STREAM frame cannot change the stream size. If this is not
respected, a CONNECTION_CLOSE with FINAL_SIZE_ERROR will be emitted as
specified in the RFC 9000.
MINOR: mux-quic: rename qcs flag FIN_RECV to SIZE_KNOWN
Rename QC_SF_FIN_RECV to the more generic name QC_SF_SIZE_KNOWN. This
better align with the QUIC RFC 9000 which uses the "Size Known" state
definition. This change is purely cosmetic.
Review the whole API used to access/instantiate qcs.
A public function qcc_open_stream_local() is available to the
application protocol layer. It allows to easily opening a local stream.
The ID is automatically attributed to the next one available.
For remote streams, qcc_open_stream_remote() has been implemented. It
will automatically take care of allocating streams in a linear way
according to the ID. This function is called via qcc_get_qcs() which can
be used for each qcc_recv*() operations. For the moment, it is only used
for STREAM frames via qcc_recv(), but soon it will be implemented for
other frames types which can also be used to open a new stream.
qcs_new() and qcs_free() has been restricted to the MUX QUIC only as
they are now reserved for internal usage.
This change is a pure refactoring and should not have any noticeable
impact. It clarifies the developer intent and help to ensure that a
stream is not automatically opened when not desired.
Implement a function <qcs_sc> to easily access to the stconn associated
with a QCS. This takes care of qcs.sd which may be NULL, for example for
unidirectional streams.
It is expected that in the future when implementing
STOP_SENDING/RESET_STREAM, stconn must be notify about the event. This
accessor will allow to easily test if the stconn is instantiated or not.
<qcc.cl_bidi_r> is used to implement STREAM ID flow control enforcement.
Move it with all fields related to this operation and separated from MAX
STREAM DATA calcul.
MEDIUM: mworker: set the iocb of the socketpair without using fd_insert()
The worker was previously changing the iocb of the socketpair in the
worker by mworker_accept_wrapper(). However, it was done using
fd_insert() instead of changing directly the callback in the
fdtab[].iocb pointer.
This patch cleans up this by part by removing fd_insert().
It also stops setting tid_bit on the thread mask, the socketpair will be
handled by any thread from now.
MINOR: proxy: use tg->threads_enabled in hard_stop() to detect stopped threads
Let's rely on tg->threads_enabled there to detect running threads. We
should probably have a dedicated function for this in order to simplify
the code and avoid the risk of using the wrong group ID.
BUG/MEDIUM: thread: check stopping thread against local bit and not global one
Commit ef422ced9 ("MEDIUM: thread: make stopping_threads per-group and add
stopping_tgroups") moved the stopping_threads mask to per-group, but one
test in the loop preserved its global value instead, resulting in stopping
threads never sleeping on stop and eating 100% CPU until all were stopped.
BUG/MEDIUM: threads: fix incorrect thread group being used on soft-stop
Commit 377e37a80 ("MINOR: tinfo: add the mask of enabled threads in each
group") forgot -1 on the tgid, thus the groups was not always correctly
tested, which is visible only when running with more than one group. No
backport is needed.
Building with threads and without thread dump (e.g. macos, freebsd)
warns that thread_dump_state is unused. This happened in fact with
recentcommit 1229ef312 ("MINOR: wdt: do not rely on threads_to_dump
anymore"). The solution would be to mark it unused, but after a
second thought, it can be convenient to keep it exported to help
debug crashes, so let's export it again. It's just not referenced in
include files since it's not needed outside.
MEDIUM: debug: make the thread dumper not rely on a thread mask anymore
The thread mask is too short to dump more than 64 bits. Thus here we're
using a different approach with two counters, one for the next thread ID
to dump (which always exists, as it's looked up), and the second one for
the number of threads done dumping. This allows to dump threads in ascending
order then to let them wait for all others to be done, then to leave without
the risk of an overlapping dump until the done count is null again.
This allows to remove threads_to_dump which was the last non-FD variable
using a global thread mask.
MINOR: wdt: do not rely on threads_to_dump anymore
This flag is not needed anymore as we're already marking the waiting
threads as harmless, thus the thread's bit is already covered by this
information. The variable was unexported.
MINOR: debug: mark oneself harmless while waiting for threads to finish
The debug_handler() function waits for other threads to join, but does
not mark itself as harmless, so if at the same time another thread tries
to isolate, this may deadlock. In practice this does not happen as the
signal is received during epoll_wait() hence under harmless mode, but
it can possibly arrive under other conditions.
In order to improve this, while waiting for other threads to join, we're
now marking the current thread as harmless, as it's doing nothing but
waiting for the other ones. This way another harmless waiter will be able
to proceed. It's valid to do this since we're not doing anything else in
this loop.
One improvement could be to also check for the thread being idle and
marking it idle in addition to harmless, so that it can even release a
full isolation requester. But that really doesn't look worth it.
MINOR: thread: add is_thread_harmless() to know if a thread already is harmless
The harmless status is not re-entrant, so sometimes for signal handling
it can be useful to know if we're already harmless or not. Let's add a
function doing that, and make the debugger use it instead of manipulating
the harmless mask.
projects / thirdparty / haproxy.git / log
