The current tasklet_wakeup() call relies on tasklet_wakeup_on(tl->tid),
which was already quite ambiguous till now due to the sole reliance on
tid being negative or not to decide to run locally, but it no longer
works correctly if used to wake tasks up since the new set of possible
negative values for ->tid (particularly if some code calls
__tasklet_wakeup_on() on a task as is done in task_instant_wakeup()).
The problem is that it is not possible in the current API to explicitly
say that we want a task/tasklet to run locally or remotely without having
to play games with a thread number. The chosen approach to address this
is to change tasklet_wakeup_on() to always be remote and have
tasklet_wakeup_here() which will always be local, with tasklet_wakeup()
choosing one or the other depending on the tid, for backwards compat
only.
This patch implements tasklet_wakeup_here() to __tasklet_wakeup_here()
that reimplement the part of __tasklet_wakeup_on() that used to deal
with the local thread only (negative tid). No other change was made.
For now it remains unused.
The doc was updated.
The checks on TH_FL_TASK_PROFILING that are used to decide whether or not
to set t->wake_date from now_mono_time() used to be made in callers of
__tasklet_wakeup_on() and __tasklet_wakeup_after(), but not only this
needlessly inflates code by placing this in every caller (~4kB), it also
renders the design fragile since each caller needs to blindly copy-paste
that statement.
Let's move the operation in the callees instead. As a bonus, it allows
to check the flag on the target thread and not on the calling thread
(which was arguably a bug though without a noticeable effect since for
now profiling is for all threads or none).
quic_pacing.c is missing a number of include files that it got by chance
through task.h, resulting in build breakage as soon as that one gets
cleaned up. Let's add api.h and activity.h that are needed. No backport
is needed.
In 1.3.11, 19 years ago, commit bdefc513a0 ("[BUG] fix null timeouts in
*poll-based pollers") addressed an issue where some wakeup times could
sometimes be rounded to less than one millisecond (by then they were
calculated on timeval), and would make the poller wake up too early and
loop with a timeout of zero. The solution used by then consisted in
always adding 1 to the wait delay so that poll() was never called with
a null timeout.
Nowadays our internal wakeup delays are in milliseconds so we cannot wake
too early, all the timeout calculation was moved to compute_poll_timeout()
which has a specific check for expired next wakeup event, so we cannot
even have a null timeout as a result of a real delay calculation by
accident. Yet, it's clearly visible with strace thats a task created
with an interval of 10ms results in a poll timeout of 11ms, causing some
small time drift in periodic wakeups.
Let's just now drop this "+1" which is no longer needed nor relevant and
only causes wrong delays to be calculated. Now creating a time-printing
task results in correct delays passed to poll() and measured intervals
around:
- ~10.3ms interval for 10ms
- ~100.5ms for 100ms
- ~1001ms for 1000ms
E.g:
$ socat - /tmp/sock1 <<< "expert-mode on;debug dev sched task print=1 inter=10 count=1"
(...)
17:58:05.191885 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=44590744}) = 0
17:58:05.191919 epoll_wait(4, [], 200, 10) = 0
17:58:05.202215 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=44601494}) = 0
17:58:05.202237 write(1, "task 0x3aeeb080: time_ms=3553"..., 42task 0x3aeeb080: time_ms=355304053.757383
) = 42
17:58:05.202253 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=44610199}) = 0
17:58:05.202265 epoll_wait(4, [], 200, 10) = 0
17:58:05.212579 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=44631754}) = 0
17:58:05.212639 write(1, "task 0x3aeeb080: time_ms=3553"..., 42task 0x3aeeb080: time_ms=355304064.157626
) = 42
These delays with longer sleeps are entirely on the system side, most
likely due to the CPU switching to low-power for such long delays (tests
run on a laptop).
There is no reason to backport this fix, though it shouldn't hurt either.
Passing "print=1" for a periodic task will cause it to print the exact
monotonic time at each wakeup to stdout and do nothing else. This is
convenient to watch wakeup delay drifts.
Since commit 0988b9c773 ("MEDIUM: tasks: Remove the per-thread group
wait queue") in 3.5-dev, a task's tid may be as negative as -MAX_THREAD-1
and not just -1, so we must accept this when trying to check if a pointer
looks like a valid task.
No backport is needed.
Add a QUIC regression test for an HTTP/3 request body without
Content-Length forwarded to an HTTP/1 backend.
The client side sends a chunked HTTP/1 request to a first HAProxy
instance, which forwards it to a second HAProxy instance over QUIC/H3.
The second instance then forwards it to a plain HTTP/1 server. This
exercises the H3 frontend to H1 backend path with an unknown request
body length.
When an HTTP/3 request carries DATA frames without a Content-Length
header, the H3 mux updates the stream endpoint known input payload
length so the stream layer can pass this information to the output mux.
The current code assigns h3s->data_len to qcs->sd->kip. However,
h3s->data_len is cumulative, while sedesc->kip is an incremental value:
it is moved to the opposite side as kop and then consumed by the output
mux. With multiple DATA frames, the H1 output mux may therefore announce
chunk sizes based on the total body length received so far instead of
the next payload length.
For an H3-to-H1 request without Content-Length, this can produce
malformed chunked encoding on the backend connection. A backend HTTP/1
parser may then reject the request, and HAProxy can return a 500 to the
client.
Fix this by incrementing qcs->sd->kip with the current DATA frame length
instead of assigning the cumulative body length.
This should be backported up to 3.3.
The `core.httpclient()` and `core.tcp()` functions aren't actually
available in the init context, as they require the event loop to be set
up.
As such, remove "init" form the list of contexts for those functions.
It probably makes sense to backport this to previous versions.
Fixes: #3420
Signed-off-by: Thayne McCombs <astrothayne@gmail.com>
When using QUIC on the backend side, transcoding of a large HTTP
response may cause the rxbuf to wrap. This patch introduces buffer
wrapping support for the HTTP/0.9 transcoder. This is similar to what is
already implemented in HTTP/3 layer.
This should be backported up to 3.3.
When dealing with large responses, QUIC MUX demux buffer may be full. In
this case, QC_SF_DEM_FULL flag must be set to pause transcoding. This
patch implements it for HTTP/0.9 response transcoding, similarly to what
HTTP/3 already provides. This is a backend side fix.
This should be backported up to 3.3.
When the Lua HTTP applet was migrated to the new API to use its own buffers,
a regression was introduced. The EOS flag at the end of the response was no
longer set. While it is not an issue when the response length is known
(because of a content-length or a transfer-encoding header), it is an issue
for responses with an unkown payload size. For the stconn and the stream, in
that case, the EOS is used to detect the end of the response. Without this
info, the stream remains blocked.
To fix the issue, the EOS flag is now set as expected on the applet.
This patch should fix the issue #3422. It must be backport as far as 3.3.
HTTP/0.9 transcoder is minimal. In particular, it did not checked if the
HTX payload length was unknown. In this case, the stream shutdown is the
normal termination signal. As this condition was not reported to the
MUX, the stream would be closed via a RESET_STREAM during the stream
shut callback invokation.
Fix this by properly inspecting HTX response line prior to generating
the HTTP/0.9 response. If flag HTX_SL_F_XFER_LEN is not set, correctly
convert it to QCS flag QC_SF_UNKNOWN_PL_LENGTH. This ensures that MUX
will use a FIN signal instead of a RESET_STREAM frame when shut is
called by the upper stream layer.
This procedure is already implemented by HTTP/3 transcoder.
This bug was detected with haterm, because contrary to httpterm the
latter does not honour Connection keep-alive header in case of HTTP/1.0.
Thus connection close mode is used and no content-length is added.
This must be backported up to 2.8.
HTTP/0.9 is a simple protocol. The response only contains the body
without any status line nor header. An HTX start line must be built when
transcoding the message for haproxy stream layer on the first invocation
of rcv_buf() callback.
Previously, this condition was detected by using an access to the stream
object. However, it's possible to rely only on the QCS by checking the
value from <rx.offset> field. This is a better solution which completely
remove the superfluous dependency between hq-interop and the stream
layer.
hq-interop request parser is minimal. It simply extracts a method and a
path until the whitespace delimiter. Parsing is interrupted if the
delimiter cannot be found, and MUX is responsible to reinvoke it later
with new content added.
This patch adjusts hq-interop parsing in case of a missing delimiter.
Now it also checks if there is space remaining in the buffer. If this is
not the case, it returns a fatal error as parsing cannot be completed at
all.
This change has the side effect of preventing a BUG_ON() crash in MUX :
in case of a truncated parsing, qcs_transfer_rx_data() may be used to
realign content from the next buffer. However this function explicitely
forbids to be called with a full buffer as it could do nothing in this
case, hence this BUG_ON() to ensure parsing is never fully blocked.
The impact of this bug remains low despite the potential BUG_ON() crash.
This is because hq-interop is only used for QUIC debugging purpose and
should not be activated in production. HTTP/3 layer is immune as it
already ensures that frame length is never bigger than a buffer size
(except for DATA frames which can be parsed in a streaming mode).
Thanks to BeaCox <root@beacox.space> for having reported us this issue.
This should be backported up to 2.6. From 3.3, qcm_stream_rx_bufsz() is
using the older "qmux" prefix and must be renamed. Also, this function
does not exists in 3.0 and older so the test must be adjusted there as
well.
The FCGI demux record length field (drl) is uint16_t. In the
ignore_record path, the expression "fconn->drl += fconn->drp" overflows
to 0 when contentLength=65535 and paddingLength>=1. This causes the
state machine to consider the record complete without consuming any
buffer data. The remaining buffer contents are then parsed as new FCGI
record headers.
The same drl+=drp pattern at lines 2382/2418/2475 is not affected
because drl is guaranteed to be 0 at those points (all content bytes
are consumed before reaching end_transfer).
Widen drl from uint16_t to uint32_t so that the addition of drp
(uint8_t, max 255) cannot overflow.
Reported-by: Tristan (@TristanInSec)
For set-headers-bin and add-header-bin actions, the new HTX headers to add
or set are extracted from the result of a sample expression evaluation. This
result must be copied because it is based a static trash chunk and internal
functions manipulating HTX messages also use static chunks. So to not
overwrite the sample expression, for instance because a defrag is performed,
it must be copied into a dynamically allocated chunk.
It is also important because the sample expression may be based on part of
the HTX message itself.
This patch must be backported to 3.4.
QCS uses a multiple Rx buffer for incoming data to decode. However this
may cause the app proto layer to block on decoding if some data are
splitted across two buffers. In this case qcs_transfer_rx_data() is used
to move the data in the same current buffer.
However, this function contains a bug as the next Rx buffer is not
reinserted as expected in the QCS tree after the move operation. This
can cause data loss which will cause a transfer freeze. This may affect
POST requests on the frontend side as well as most backend transfers.
This patch fixes the reinsert operation. It is now performed as expected
if the next rxbuf is not fully truncated. In the opposite case, the
rxbuf can simply be freed completely.
This must be backported up to 3.2.
hpack_dht_insert() has three call sites for hpack_dht_defrag(). Two of
them (lines 293 and 306) correctly check for a NULL return and bail out
with -1. The third (line 353, data-space defrag path) assigns the return
value to dht and immediately dereferences it without a NULL check.
When pool_head_hpack_tbl is exhausted, hpack_dht_alloc() returns NULL,
hpack_dht_defrag() propagates it, and line 354 dereferences NULL+0x0a
(offsetof wrap), crashing the worker with SIGSEGV.
Add a NULL check consistent with the two other call sites.
This must be backported to all stable versions.
Reported-by: Tristan (@TristanInSec)
When 0RTT is enabled, a temporary buffer for early data is used. We read
from it first when the mux asks for data, and then we free it when it is
empty, but that is not right, because maybe we have more early data to
receive, and then we no longer have any buffer to store them, and that
will eventually end up with the connection closed in error.
To fix that, as long as we haven't received all the early data yet, just
reset the buffer, instead of freeing it.
This should fix github issue #3416
This should be backported up to 2.8.
QUIC flow control on bidirectional streams ensure that the peer cannot
emit more than what haproxy has allowed, which guarantees that buffering
is under controlled on the receiver side.
This limit is first announced on the transport parameter via
initial_max_stream_data_bidi_remote which is derived from configuration
value. QUIC MUX calculation for its streams is then directly based on
the transport parameter.
This mechanism works as expected on the frontend side, as in this case
all exchanges occur on remote streams opened by the opposite side.
However, this is not working as expected on the backend side, as in this
case transfers occur on streams opened locally by haproxy as the client.
Thus, configuration has no impact on backend side rxbuf which remains
set to a single buffer, causing important latency when retrieving large
objects.
This patch removes this limitation on the backend side by adjusting
quic_transport_params_init(). If <server> parameter is false, limitation
is set for initial_max_stream_data_bidi_local TP.
This must be backported up to 3.3.
The file specified by 'h1-case-adjust-file' directive is only loaded during
post-parsing. However when a relative path is used, the corresponding
absoulte path was not resolve during parsing. So the file could be loaded
relatively from the wrong location leading to a configuration error. It may
happen if several configuration files are used or if several
"default-config" are used. The last "default" location was always used.
To fix the issue, the absolute path of the file is now resolved when the
directive is parsed.
This patch should fix the issue #3415. It must be backported to all
versions.
with L7 retries are configured, when the max number of retries is reached
the error must be reported to the client. However, when it was an abort on a
reused connections, the client connection is silently closed. While it is
expected without L7 retries, to let the client retries on its own, it is
unexepcted with L7 retries.
So let's fix it by ignoring the SF_SRV_REUSED flag on the stream when a L7
retry fails. This way, a 502/425 will be reported to the client.
This patch should help to fix the issue #3414. It must be backported to all
supported versions.
A memset used for debug was left when "keep-query" option was added. Let's
remove it. This bug should be harmless but it consumes extra CPU for
nothing.
This patch should be backported as far as 3.2.
Whne the endpoint is shut (an applet or a mux), at least one of the SHW
flags must be set (SE_SHW_SILENT or SE_SHW_NORMAL). It is mandatory for
muxes to perform the shutdown. Otherwise, the shutdown could be ignored.
So let's add a BUG_ON() on it to be sure this never happen.
This patch documents the behavior where the internal HTTP client sets
the response status to 0 when an error is encountered by the stream
(SF_ERR_MASK).
This allows users to distinguish between an HTTP status code returned
by a remote server and an internal error generated by HAProxy (e.g.
connection timeout, connection refused, etc.).
With the httpclient it's difficult to get if the HTTP status code was
returned by the actual server or if it's the internal proxy that
generate the error.
This patch changes the behavior by setting the status to 0 when an error
is get by the stream.
There were already valid cases when the status was 0 on some error, so
that should not really change the error path in the scripts.
Refactor the Lua HTTP client to defer initialization. core.httpclient()
no longer initializes the internal HTTP client immediately. Instead,
initialization now occurs within hlua_httpclient_send() when a request
method (e.g., get, put, head) is invoked.
The HTTPClient class now serves as a factory for accessing methods, while
a new class, HTTPClientRequest, has been introduced to represent individual
requests and manage the HTTP client lifecycle.
This change allows multiple requests to be executed using a single
HTTP client instance:
local hc = core.httpclient()
local res1 = hc:get({url = "...", headers = ...})
local res2 = hc:post({url = "...", headers = ...})
local res3 = hc:put({url = "...", headers = ...})
This refactor maintains backward compatibility, as existing scripts that
instantiate a new core.httpclient() for every request will continue to
work as expected.
Move the lua httpclient code from hlua.c to http_client.c
The code is almost the same but the registering of the class which is
done in hlua_http_client_init_state(), from REGISTER_HLUA_STATE_INIT()
check_args() calls have been replaced by hlua_check_args().
hlua_httpclient_destroy_all() is exported so it can be called in hlua.c.
hlua_httpclient_table_to_hdrs() is made static.
hlua_pusherror() and check_args() are being exported.
check_args() is now a macro to hlua_check_args() so it's not confusing
when called outside hlua.c.
When a dynamic server is added with consistent hash balancing on the
backend, its lb_nodes elements are allocated and associated with a
calculated server key. This operation is performed in add server handler
via srv_alloc_lb(). By default, the server key is based on its ID.
However, automatic server ID is calculated later in add server handler,
which means the initial lb_nodes are not valid.
This could cause load balacing issue but in fact this is not directly
visible as the server key is recalculated when the dynamic server is
enabled via chash_queue_dequeue_srv() : all server lb_nodes are dequeued
and requeued with the now proper key.
Thus, "add server" handler must be corrected as it is buggy when
considering it alone. The simplest solution of the current patch is to
initialize server ID before srv_alloc_lb() is invoked. There is no issue
as handler runs under thread isolation so there is no risk of multiple
servers manipulating the same ID. Server insertion in proxy ID tree is
still performed at the end of the handler when all fallible operation
are completed.
The fact that server key is recalculated when the server is set to ready
state is a side effect of the following patch which was introduced in
3.0. What this means though is that users of older releases are facing a
bigger issue, with load-balancing not working as expected. Thus,
this patch is even more crucial for 2.8 and older releases.
faa8c3e024
MEDIUM: lb-chash: Deterministic node hashes based on server address
This should fix github issue #3413. Thanks to Joao Morais for is
analysis on the problem.
This must be backported to all stable releases.
When we receive a PUSH_PROMISE frame while we don't expect it, flag it
as a connection error, do not just set ret to H3_ERR_ID_ERROR, as it
would just be considered the number of bytes we read, and could lead to
random corruption. This should only happen with backend connections.
This should be backported whenever commit 4a8bb2fe5 is backported.
Now that there is no longer a shared wake queue, chances are if a shared task
is scheduled, it will always end up on the same thread. In
wake_expired_tasks(), when a task has to be waken up, randomly look to
three other threads, and if the runqueue of the current thread is at least
two time bigger than the runqueue of one of the other threads, then give
that task to that thread, so that our load gets reduced.
If we're giving the task to another thread, then we have to add the
TASK_RUNNING flag until we waked it up, otherwise the other thread could
just run it, if it gets waken up from another path, and free it while
we're still not done with it.
2 times has been chosen somewhat arbitrarily, and may be tweaked at a
later date if deemed not optimal.
Modify task_instant_wakeup() to use __task_set_state_and_tid().
It uses the new ownership behavior, but that's okay because
task_instant_wakeup() was not used anywhere.
Totally remove the per-thread group wait queue. This was potentially a
source of contention, because there were only a global lock for all
those wait queues.
Instead, for shared tasks, there is now the concept of ownership for the
task. When a task is in the wait queue, run queue, or is running on that
particular thread, the task's tid is set to -2 - thread_tid, and only
that thread will be responsible for it until it is no longer running,
and in none of its queue.
When a shared task is scheduled to be run at a later time, if its
current tid is -1, then the current thread will take ownership, and put
it in its own wait queue. If it is already owned, then TASK_WOKEN_WQ is
added to the task's state, and a task_wakeup() is done, so that the
owner thread will add it in its wait queue.
If there is any owner, then a task_wakeup() will just add the task to
the owner's runqueue, otherwise the current thread will become the
owner.
Introduce a new function, __task_get_current_owner, that returns the
owner of a task based on its current tid.
-1 means there is no current owner, otherwise either the tid is >= 0, in
which case it will just return it, or it's < -1, in which case it will
return -2 - tid, the tid of the thread with the current ownership.
Introduce __task_get_new_tid_field(), that provides the tid to be used
for a task.
For shared task, to mark temporary ownership of a task, instead of -1,
the tid will be set to -2-tid, tid being the tid of the current thread.
Introduce a new function, __task_set_state_and_tid, that atomically can
set a task's state and its tid. This will be used later, as the tid will
be used to indicate task ownership even for shared tasks.
This Lua script automates dns-01 ACME challenges using the Gandi LiveDNS
API v5. It subscribes to the ACME_DEPLOY event to set the required
_acme-challenge TXT record via the Gandi REST API, signals HAProxy that
the challenge is ready using ACME.challenge_ready(), then cleans up the
TXT record once the certificate is issued on ACME_NEWCERT.
The API key is read from the GANDI_API_KEY environment variable at
startup. Zone discovery is automatic: the script probes parent zones
from longest to shortest until Gandi accepts the record, which handles
both apex and wildcard certificates transparently.
Add EVENT_HDL_SUB_ACME_DEPLOY to the ACME family. It is published in
the dns-01 challenge path after the TXT record information has been
prepared, carrying the certificate store name, domain, account
thumbprint, dns_record value, and optionally the provider and vars
strings.
Lua subscribers using core.event_sub() receive the event data as an
AcmeEvent object, which is the same class used for ACME_NEWCERT and
carries the fields relevant to the event type.
Add a new EVENT_HDL_SUB_ACME_NEWCERT event type in the ACME family.
It is published after a new certificate has been successfully fetched
and installed. The event carries the certificate store name, allowing
subscribers to act on newly available certificates.
Lua subscribers using core.event_sub() receive the event data as an
AcmeEvent object with a crtname field containing the certificate store
name.
Using ha_diag_warning() to report the number of threads created resulted
in warnings being counted and possibly an error being fired when combined
with -dW:
$ printf "global\nstats socket /tmp/sock1\n" | ./haproxy -dD -dW -c -f /dev/stdin; echo $?
[NOTICE] (10406) : haproxy version is 3.5-dev0-5091ac-35
[NOTICE] (10406) : path to executable is ./haproxy
[DIAG] (10406) : Created 20 threads split into 2 groups
[ALERT] (10406) : Some warnings were found and 'zero-warning' is set. Aborting.
1
Now that we have ha_diag_notice(), let's use it:
$ printf "global\nstats socket /tmp/sock1\n" | ./haproxy -dD -dW -c -f /dev/stdin; echo $?
[DIAG] (10513) : Created 20 threads split into 2 groups
0
It would make sense to backport this to 3.2 because it helps validate configs
against diag warnings without triggering a false positive. It depends on
this previous patch:
MINOR: errors: add ha_diag_notice() to report diag-level notifications
Right now the only way to report info that is only displayed in diag
mode with -dD is to use ha_diag_warning(). The problem is that this is
then counted as a warning and may result in errors when combined with
-dW, as happens for the CPU topology info:
$ printf "global\nstats socket /tmp/sock1\n" | ./haproxy -dD -dW -c -f /dev/stdin; echo $?
[NOTICE] (10406) : haproxy version is 3.5-dev0-5091ac-35
[NOTICE] (10406) : path to executable is ./haproxy
[DIAG] (10406) : Created 20 threads split into 2 groups
[ALERT] (10406) : Some warnings were found and 'zero-warning' is set. Aborting.
1
We need another level. This commit introduces ha_diag_notice() which only
emits a notification that doesn't count as a warning. Note that we could
even introduce an info level and revisit various messages so that notice
only reports certain events while info is for anything (like versions
above). That could be a future improvement.
