Released version 2.8-dev7 with the following main changes :
- BUG/MINOR: stats: Don't replace sc_shutr() by SE_FL_EOS flag yet
- BUG/MEDIUM: mux-h2: Be able to detect connection error during handshake
- BUG/MINOR: quic: Missing padding in very short probe packets
- MINOR: proxy/pool: prevent unnecessary calls to pool_gc()
- CLEANUP: proxy: remove stop_time related dead code
- DOC/MINOR: reformat configuration.txt's "quoting and escaping" table
- MINOR: http_fetch: Add support for empty delim in url_param
- MINOR: http_fetch: add case insensitive support for smp_fetch_url_param
- MINOR: http_fetch: Add case-insensitive argument for url_param/urlp_val
- REGTESTS : Add test support for case insentitive for url_param
- BUG/MEDIUM: proxy/sktable: prevent watchdog trigger on soft-stop
- BUG/MINOR: backend: make be_usable_srv() consistent when stopping
- BUG/MINOR: ssl: Remove dead code in cli_parse_update_ocsp_response
- BUG/MINOR: ssl: Fix potential leak in cli_parse_update_ocsp_response
- BUG/MINOR: ssl: ssl-(min|max)-ver parameter not duplicated for bundles in crt-list
- BUG/MINOR: quic: Wrong use of now_ms timestamps (cubic algo)
- MINOR: quic: Add recovery related information to "show quic"
- BUG/MINOR: quic: Wrong use of now_ms timestamps (newreno algo)
- BUG/MINOR: quic: Missing max_idle_timeout initialization for the connection
- MINOR: quic: Implement cubic state trace callback
- MINOR: quic: Adjustments for generic control congestion traces
- MINOR: quic: Traces adjustments at proto level.
- MEDIUM: quic: Ack delay implementation
- BUG/MINOR: quic: Wrong rtt variance computing
- MINOR: cli: support filtering on FD types in "show fd"
- MINOR: quic: Add a fake congestion control algorithm named "nocc"
- CI: run smoke tests on config syntax to check memory related issues
- CLEANUP: assorted typo fixes in the code and comments
- CI: exclude doc/{design-thoughts,internals} from spell check
- BUG/MINOR: quic: Remaining useless statements in cubic slow start callback
- BUG/MINOR: quic: Cubic congestion control window may wrap
- MINOR: quic: Add missing traces in cubic algorithm implementation
- BUG/MAJOR: quic: Congestion algorithms states shared between the connection
- BUG/MINOR: ssl: Undefined reference when building with OPENSSL_NO_DEPRECATED
- BUG/MINOR: quic: Remove useless BUG_ON() in newreno and cubic algo implementation
- MINOR: http-act: emit a warning when a header field name contains forbidden chars
- DOC: config: strict-sni allows to start without certificate
- MINOR: quic: Add trace to debug idle timer task issues
- BUG/MINOR: quic: Unexpected connection closures upon idle timer task execution
- BUG/MINOR: quic: Wrong idle timer expiration (during 20s)
- BUILD: quic: 32bits compilation issue in cli_io_handler_dump_quic()
- BUG/MINOR: quic: Possible wrong PTO computing
- BUG/MINOR: tcpcheck: Be able to expect an empty response
- BUG/MEDIUM: stconn: Add a missing return statement in sc_app_shutr()
- BUG/MINOR: stream: Fix test on channels flags to set clientfin/serverfin touts
- MINOR: applet: Uninline appctx_free()
- MEDIUM: applet/trace: Register a new trace source with its events
- CLEANUP: stconn: Remove remaining debug messages
- BUG/MEDIUM: channel: Improve reports for shut in co_getblk()
- BUG/MEDIUM: dns: Properly handle error when a response consumed
- MINOR: stconn: Remove unecessary test on SE_FL_EOS before receiving data
- MINOR: stconn/channel: Move CF_READ_DONTWAIT into the SC and rename it
- MINOR: stconn/channel: Move CF_SEND_DONTWAIT into the SC and rename it
- MINOR: stconn/channel: Move CF_NEVER_WAIT into the SC and rename it
- MINOR: stconn/channel: Move CF_EXPECT_MORE into the SC and rename it
- MINOR: mux-pt: Report end-of-input with the end-of-stream after a read
- BUG/MINOR: mux-h1: Properly report EOI/ERROR on read0 in h1_rcv_pipe()
- CLEANUP: mux-h1/mux-pt: Remove useless test on SE_FL_SHR/SE_FL_SHW flags
- MINOR: mux-h1: Report an error to the SE descriptor on truncated message
- MINOR: stconn: Always ack EOS at the end of sc_conn_recv()
- MINOR: stconn/applet: Handle EOI in the applet .wake callback function
- MINOR: applet: No longer set EOI on the SC
- MINOR: stconn/applet: Handle EOS in the applet .wake callback function
- MEDIUM: cache: Use the sedesc to report and detect end of processing
- MEDIUM: cli: Use the sedesc to report and detect end of processing
- MINOR: dns: Remove the test on the opposite SC state to send requests
- MEDIUM: dns: Use the sedesc to report and detect end of processing
- MEDIUM: spoe: Use the sedesc to report and detect end of processing
- MEDIUM: hlua/applet: Use the sedesc to report and detect end of processing
- MEDIUM: log: Use the sedesc to report and detect end of processing
- MEDIUM: peers: Use the sedesc to report and detect end of processing
- MINOR: sink: Remove the tests on the opposite SC state to process messages
- MEDIUM: sink: Use the sedesc to report and detect end of processing
- MEDIUM: stats: Use the sedesc to report and detect end of processing
- MEDIUM: promex: Use the sedesc to report and detect end of processing
- MEDIUM: http_client: Use the sedesc to report and detect end of processing
- MINOR: stconn/channel: Move CF_EOI into the SC and rename it
- MEDIUM: tree-wide: Move flags about shut from the channel to the SC
- MINOR: tree-wide: Simplifiy some tests on SHUT flags by accessing SCs directly
- MINOR: stconn/applet: Add BUG_ON_HOT() to be sure SE_FL_EOS is never set alone
- MINOR: server: add SRV_F_DELETED flag
- BUG/MINOR: server/del: fix srv->next pointer consistency
- BUG/MINOR: stats: properly handle server stats dumping resumption
- BUG/MINOR: sink: free forward_px on deinit()
- BUG/MINOR: log: free log forward proxies on deinit()
- MINOR: server: always call ssl->destroy_srv when available
- MINOR: server: correctly free servers on deinit()
- BUG/MINOR: hlua: hook yield does not behave as expected
- MINOR: hlua: properly handle hlua_process_task HLUA_E_ETMOUT
- BUG/MINOR: hlua: enforce proper running context for register_x functions
- MINOR: hlua: Fix two functions that return nothing useful
- MEDIUM: hlua: Dynamic list of frontend/backend in Lua
- MINOR: hlua_fcn: alternative to old proxy and server attributes
- MEDIUM: hlua_fcn: dynamic server iteration and indexing
- MEDIUM: hlua_fcn/api: remove some old server and proxy attributes
- CLEANUP: hlua: fix conflicting comment in hlua_ctx_destroy()
- MINOR: hlua: add simple hlua reference handling API
- MINOR: hlua: fix return type for hlua_checkfunction() and hlua_checktable()
- BUG/MINOR: hlua: fix reference leak in core.register_task()
- BUG/MINOR: hlua: fix reference leak in hlua_post_init_state()
- BUG/MINOR: hlua: prevent function and table reference leaks on errors
- CLEANUP: hlua: use hlua_ref() instead of luaL_ref()
- CLEANUP: hlua: use hlua_pushref() instead of lua_rawgeti()
- CLEANUP: hlua: use hlua_unref() instead of luaL_unref()
- MINOR: hlua: simplify lua locking
- BUG/MEDIUM: hlua: prevent deadlocks with main lua lock
- MINOR: hlua_fcn: add server->get_rid() method
- MINOR: hlua: support for optional arguments to core.register_task()
- DOC: lua: silence "literal block ends without a blank line" Sphinx warnings
- DOC: lua: silence "Unexpected indentation" Sphinx warnings
- BUG/MINOR: event_hdl: fix rid storage type
- BUG/MINOR: event_hdl: make event_hdl_subscribe thread-safe
- MINOR: event_hdl: global sublist management clarification
- BUG/MEDIUM: event_hdl: clean soft-stop handling
- BUG/MEDIUM: event_hdl: fix async data refcount issue
- MINOR: event_hdl: normal tasks support for advanced async mode
- MINOR: event_hdl: add event_hdl_async_equeue_isempty() function
- MINOR: event_hdl: add event_hdl_async_equeue_size() function
- MINOR: event_hdl: pause/resume for subscriptions
- MINOR: proxy: add findserver_unique_id() and findserver_unique_name()
- MEDIUM: hlua/event_hdl: initial support for event handlers
- MINOR: hlua/event_hdl: per-server event subscription
- EXAMPLES: add basic event_hdl lua example script
- MINOR: http-ana: Add a HTTP_MSGF flag to state the Expect header was checked
- BUG/MINOR: http-ana: Don't switch message to DATA when waiting for payload
- BUG/MINOR: quic: Possible crashes in qc_idle_timer_task()
- MINOR: quic: derive first DCID from client ODCID
- MINOR: quic: remove ODCID dedicated tree
- MINOR: quic: remove address concatenation to ODCID
- BUG/MINOR: mworker: unset more internal variables from program section
- BUG/MINOR: errors: invalid use of memprintf in startup_logs_init()
- MINOR: applet: Use unsafe version to get stream from SC in the trace function
- BUG/MUNOR: http-ana: Use an unsigned integer for http_msg flags
- MINOR: compression: Make compression offload a flag
- MINOR: compression: Prepare compression code for request compression
- MINOR: compression: Store algo and type for both request and response
- MINOR: compression: Count separately request and response compression
- MEDIUM: compression: Make it so we can compress requests as well.
- BUG/MINOR: lua: remove incorrect usage of strncat()
- CLEANUP: tcpcheck: remove the only occurrence of sprintf() in the code
- CLEANUP: ocsp: do no use strpcy() to copy a path!
- CLEANUP: tree-wide: remove strpcy() from constant strings
- CLEANUP: opentracing: remove the last two occurrences of strncat()
- BUILD: compiler: fix __equals_1() on older compilers
- MINOR: compiler: define a __attribute__warning() macro
- BUILD: bug.h: add a warning in the base API when unsafe functions are used
- BUG/MEDIUM: listeners: Use the right parameters for strlcpy2().
When calls to strcpy() were replaced with calls to strlcpy2(), one of them
was replaced wrong, and the source and size were inverted. Correct that.
This should fix issue #2110.
Once in a while we introduce an sprintf() or strncat() function by
accident. These ones are particularly dangerous and must never ever
be used because the only way to use them safely is at least as
complicated if not more, than their safe counterparts. By redefining
a few of these functions with an attribute_warning() we can deliver a
message to the developer who is tempted to use them. This commit does
it for strcat(), strcpy(), strncat(), sprintf(), vsprintf(). More could
come later if needed, such as strtok() and maybe a few others, but these
are less common.
__attribute__((deprecated)) is convenient to discourage from using
something deprecated, but gcc >= 4.3 provides __attribute__((warning(x)))
that allows to display a specific warning if something is used. This is
particularly convenient to give indications when some API parts need to
be adapted. Let's just define it as a macro that falls back to the older
deprecated attribute when not available.
It's supported on clang 14 as well but works differently and errors
out when redefined (while the main purpose precisely is to add such a
redefinition). Thus instead on clang we use deprecated(msg) which is
OK. See https://github.com/llvm/llvm-project/issues/56519
It appeared that __has_attribute() doesn't work on gcc 4.4 and older
because the concatenation of __has_attribute##x isn't resolved as a one
before being passed to __equals_1() which immediately concatenates it to
comma_for_one. We first need to pass it through an extra layer to resolve
this name to a value. The new version was tested with gcc 4.2 to 11.3.
This may be backported though it's pretty minor.
In flt_ot_sample_to_str() there were two occurrences of strncat() which
are used to copy N chars from the source and append a zero. For the sake
of definitely getting rid of this nasty function let's replace them by
memcpy() instead. It's worth noting that the length test there appeared
to be incorrect as it didn't make provisions for the trailing zero,
unless the size argument doesn't take it into account (seems unlikely).
Nothing was changed regarding this. If the code was good, it still is,
otherwise if it was bad it still is. At least this is more obvious now
than when using a function that needs n+1 chars to work.
These ones are genenerally harmless on modern compilers because the
compiler checks them. While gcc optimizes them away without even
referencing strcpy(), clang prefers to call strcpy(). Nevertheless they
prevent from enabling stricter checks so better remove them altogether.
They were all replaced by strlcpy2() and the size of the destination
which is always known there.
strcpy() is quite nasty but tolerable to copy constants, but here
it copies a variable path into a node in a code path that's not
trivial to follow given that it takes the node as the result of
a tree lookup. Let's get rid of it and mention where the entry
is retrieved.
As every time strncat() is used, it's wrong, and this one is no exception.
Users often think that the length applies to the destination except it
applies to the source and makes it hard to use correctly. The bug did not
have an impact because the length was preallocated from the sum of all
the individual lengths as measured by strlen() so there was no chance one
of them would change in between. But it could change in the future. Let's
fix it to use memcpy() instead for strings, or byte copies for delimiters.
No backport is needed, though it can be done if it helps to apply other
fixes.
Add code so that compression can be used for requests as well.
New compression keywords are introduced :
"direction" that specifies what we want to compress. Valid values are
"request", "response", or "both".
"type-req" and "type-res" define content-type to be compressed for
requests and responses, respectively. "type" is kept as an alias for
"type-res" for backward compatibilty.
"algo-req" specifies the compression algorithm to be used for requests.
Only one algorithm can be provided.
"algo-res" provides the list of algorithm that can be used to compress
responses. "algo" is kept as an alias for "algo-res" for backward
compatibility.
Make provision for being able to store both compression algorithms and
content-types to compress for both requests and responses. For now only
the responses one are used.
Make provision for storing the compression algorithm and the compression
context twice, one for requests, and the other for responses. Only the
response ones are used for now.
In the commit 2954bcc1e (BUG/MINOR: http-ana: Don't switch message to DATA
when waiting for payload), the HTTP message flags were extended and don't
fit anymore in an unsigned char. So, we must use an unsigned integer now. It
is not a big deal because there was already a 6-bytes hole in the structure,
just after the flags. Now, there are a 3-bytes hold before.
This patch should fix the issue #2105. It is 2.8-specific, no backport
needed.
When a trace message for an applet is dumped, if the SC exists, the stream
always exists too. There is no way to attached an applet to a health-check.
So, we can use the unsafe version __sc_strm() to get the stream.
This patch is related to #2106. Not sure it will be enough for
Coverity. However, there is no bug here.
On startup/reload, startup_logs_init() will try to export startup logs shm
filedescriptor through the internal HAPROXY_STARTUPLOGS_FD env variable.
While memprintf() is used to prepare the string to be exported via
setenv(), str_fd argument (first argument passed to memprintf()) could
be non NULL as a result of HAPROXY_STARTUPLOGS_FD env variable being
already set.
Indeed: str_fd is already used earlier in the function to store the result
of getenv("HAPROXY_STARTUPLOGS_FD").
The issue here is that memprintf() is designed to free the 'out' argument
if out != NULL, and here we don't expect str_fd to be freed since it was
provided by getenv() and would result in memory violation.
To prevent any invalid free, we must ensure that str_fd is set to NULL
prior to calling memprintf().
This must be backported in 2.7 with eba6a54cd4 ("MINOR: logs: startup-logs
can use a shm for logging the reload")
People who use HAProxy as a process 1 in containers sometimes starts
other things from the program section. This is still not recommend as
the master process has minimal features regarding process management.
Environment variables are still inherited, even internal ones.
Since 2.7, it could provoke a crash when inheriting the
HAPROXY_STARTUPLOGS_FD variable.
Note: for future releases it should be better to clean the env and sets
a list of variable to be exported. We need to determine which variables
are used by users before.
Must be backported in 2.7.
Previously, ODCID were concatenated with the client address. This was
done to prevent a collision between two endpoints which used the same
ODCID.
Thanks to the two previous patches, first connection generated CID is
now directly derived from the client ODCID using a hash function which
uses the client source address from the same purpose. Thus, it is now
unneeded to concatenate client address to <odcid> quic-conn member.
This change allows to simplify the quic_cid structure management and
reduce its size which is important as it is embedded several times in
various structures such as quic_conn and quic_rx_packet.
This should be backported up to 2.7.
First connection CID generation has been altered. It is now directly
derived from client ODCID since previous commit :
commit 162baaff7a
MINOR: quic: derive first DCID from client ODCID
This patch removes the ODCID tree which is now unneeded. On connection
lookup via CID, if a DCID is not found the hash derivation is performed
for an INITIAL/0-RTT packet only. In case a client has used multiple
times an ODCID, this will allow to retrieve our generated DCID in the
CID tree without storing the ODCID node.
The impact of this two combined patch is that it may improve slightly
haproxy memory footprint by removing a tree node from quic_conn
structure. The cpu calculation induced by hash derivation should only be
performed only a few times per connection as the client will start to
use our generated CID as soon as it received it.
This should be backported up to 2.7.
Change the generation of the first CID of a connection. It is directly
derived from the client ODCID using a 64-bits hash function. Client
address is added to avoid collision between clients which could use the
same ODCID.
For the moment, this change as no functional impact. However, it will be
directly used for the next commit to be able to remove the ODCID tree.
This should be backported up to 2.7.
This is due to this commit:
MINOR: quic: Add trace to debug idle timer task issues
where has been added without having been tested at developer level.
<qc> was dereferenced after having been released by qc_conn_release().
Set qc to NULL value after having been released to forbid its dereferencing.
Add a check for qc->idle_timer_task in the traces added by the mentionned
commit above to prevent its dereferencing if NULL.
Take the opportunity of this patch to modify trace events from
QUIC_EV_CONN_SSLALERT to QUIC_EV_CONN_IDLE_TIMER.
Must be backported to 2.6 and 2.7.
The HTTP message must remains in BODY state during the analysis, to be able
to report accurate termination state in logs. It is also important to know
the HTTP analysis is still in progress. Thus, when we are waiting for the
message payload, the message is no longer switch to DATA state. This was
used to not process "Expect: " header at each evaluation. But thanks to the
previous patch, it is no long necessary.
This patch also fixes a bug in the lua filter api. Some functions must be
called during the message analysis and not during the payload forwarding. It
is not valid to try to manipulate headers during the forward stage because
headers are already forwarded. We rely on the message state to detect
errors. So the api was unusable if a "wait-for-body" action was used.
This patch shoud fix the issue #2093. It relies on the commit:
* MINOR: http-ana: Add a HTTP_MSGF flag to state the Expect header was checked
Both must be backported as far as 2.5.
HTTP_MSGF_EXPECT_CHECKED is now set on the request message to know the
"Expect: " header was already handled, if any. The flag is set from the
moment we try to handle the header to send a "100-continue" response,
whether it was found or not.
This way, when we are waiting for the request payload, thanks to this flag,
we only try to handle "Expect: " header only once. Before it was performed
by changing the message state from BODY to DATA. But this has some side
effects and it is no accurate. So, it is better to rely on a flag to do so.
For now: basic server event handling from lua, events are printed to
STDOUT.
The idea behind this is to provide simple and easy-to-use examples
that serve as a basic introduction for lua event handler feature.
Now that event_hdl api is properly implemented in hlua, we may add the
per-server event subscription in addition to the global event
subscription.
Per-server subscription allows to be notified for events related to
single server. It is useful to track a server UP/DOWN and DEL events.
It works exactly like core.event_sub() except that the subscription
will be performed within the server dedicated subscription list instead
of the global one.
The callback function will only be called for server events affecting
the server from which the subscription was performed.
Regarding the implementation, it is pretty trivial at this point, we add
more doc than code this time.
Usage examples have been added to the (lua) documentation.
Now that the event handler API is pretty mature, we can expose it in
the lua API.
Introducing the core.event_sub(<event_types>, <cb>) lua function that
takes an array of event types <event_types> as well as a callback
function <cb> as argument.
The function returns a subscription <sub> on success.
Subscription <sub> allows you to manage the subscription from anywhere
in the script.
To this day only the sub->unsub method is implemented.
The following event types are currently supported:
- "SERVER_ADD": when a server is added
- "SERVER_DEL": when a server is removed from haproxy
- "SERVER_DOWN": server states goes from up to down
- "SERVER_UP": server states goes from down to up
As for the <cb> function: it will be called when one of the registered
event types occur. The function will be called with 3 arguments:
cb(<event>,<data>,<sub>)
<event>: event type (string) that triggered the function.
(could be any of the types used in <event_types> when registering
the subscription)
<data>: data associated with the event (specific to each event family).
For "SERVER_" family events, server details such as server name/id/proxy
will be provided.
If the server still exists (not yet deleted), a reference to the live
server is provided to spare you from an additionnal lookup if you need
to have direct access to the server from lua.
<sub> refers to the subscription. In case you need to manage it from
within an event handler.
(It refers to the same subscription that the one returned from
core.event_sub())
Subscriptions are per-thread: the thread that will be handling the
event is the one who performed the subscription using
core.event_sub() function.
Each thread treats events sequentially, it means that if you have,
let's say SERVER_UP, then SERVER_DOWN in a short timelapse, then your
cb function will first be called with SERVER_UP, and once you're done
handling the event, your function will be called again with SERVER_DOWN.
This is to ensure event consitency when it comes to logging / triggering
logic from lua.
Your lua cb function may yield if needed, but you're pleased to process
the event as fast as possible to prevent the event queue from growing up
To prevent abuses, if the event queue for the current subscription goes
over 100 unconsumed events, the subscription will pause itself
automatically for as long as it takes for your handler to catch up.
This would lead to events being missed, so a warning will be emitted in
the logs to inform you about that. This is not something you want to let
happen too often, it may indicate that you subscribed to an event that
is occurring too frequently or/and that your callback function is too
slow to keep up the pace and you should review it.
If you want to do some parallel processing because your callback
functions are slow: you might want to create subtasks from lua using
core.register_task() from within your callback function to perform the
heavy job in a dedicated task and allow remaining events to be processed
more quickly.
Please check the lua documentation for more information.
Adding alternative findserver() functions to be able to perform an
unique match based on name or puid and by leveraging revision id (rid)
to make sure the function won't match with a new server reusing the
same name or puid of the "potentially deleted" server we were initially
looking for.
For example, if you were in the position of finding a server based on
a given name provided to you by a different context:
Since dynamic servers were implemented, between the time the name was
picked and the time you will perform the findserver() call some dynamic
server deletion/additions could've been performed in the mean time.
In such cases, findserver() could return a new server that re-uses the
name of a previously deleted server. Depending on your needs, it could
be perfectly fine, but there are some cases where you want to lookup
the original server that was provided to you (if it still exists).
While working on event handling from lua, the need for a pause/resume
function to temporarily disable a subscription was raised.
We solve this by introducing the EHDL_SUB_F_PAUSED flag for
subscriptions.
The flag is set via _pause() and cleared via _resume(), and it is
checked prior to notifying the subscription in publish function.
Pause and Resume functions are also available for via lookups for
identified subscriptions.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
Use event_hdl_async_equeue_size() in advanced async task handler to
get the near real-time event queue size.
By near real-time, you should understand that the queue size is not
updated during element insertion/removal, but shortly before insertion
and shortly after removal, so the size should reflect the approximate
queue size at a given time but should definitely not be used as a
unique source of truth.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
Add event_hdl_async_equeue_isempty() to check is the event queue is
empty from an advanced async task handler.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
advanced async mode (EVENT_HDL_ASYNC_TASK) provided full support for
custom tasklets registration.
Due to the similarities between tasks and tasklets, it may be useful
to use the advanced mode with an existing task (not a tasklet).
While the API did not explicitly disallow this usage, things would
get bad if we try to wakeup a task using tasklet_wakeup() for notifying
the task about new events.
To make the API support both custom tasks and tasklets, we use the
TASK_IS_TASKLET() macro to call the proper waking function depending
on the task's type:
- For tasklets: we use tasklet_wakeup()
- For tasks: we use task_wakeup()
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
In _event_hdl_publish(), when publishing an event to async handler(s),
async_data is allocated only once and then relies on a refcount
logic to reuse the same data block for multiple async event handlers.
(this allows to save significant amount of memory)
Because the refcount is first set to 0, there is a small race where
the consumers could consume async data (async data refcount reaching 0)
before publishing is actually over.
The consequence is that async data may be freed by one of the consumers
while we still rely on it within _event_hdl_publish().
This was discovered by chance when stress-testing the API with multiple
async handlers registered to the same event: some of the handlers were
notified about a new event for which the event data was already freed,
resulting in invalid reads and/or segfaults.
To fix this, we first set the refcount to 1, assuming that the
publish function relies on async_data until the publish is over.
At the end of the publish, the reference to the async data is dropped.
This way, async_data is either freed by _event_hdl_publish() itself
or by one of the consumers, depending on who is the last one relying
on it.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
soft-stop was not explicitly handled in event_hdl API.
Because of this, event_hdl was causing some leaks on deinit paths.
Moreover, a task responsible for handling events could require some
additional cleanups (ie: advanced async task), and as the task was not
protected against abort when soft-stopping, such cleanup could not be
performed unless the task itself implements the required protections,
which is not optimal.
Consider this new approach:
'jobs' global variable is incremented whenever an async subscription is
created to prevent the related task from being aborted before the task
acknowledges the final END event.
Once the END event is acknowledged and freed by the task, the 'jobs'
variable is decremented, and the deinit process may continue (including
the abortion of remaining tasks not guarded by the 'jobs' variable).
To do this, a new global mt_list is required: known_event_hdl_sub_list
This list tracks the known (initialized) subscription lists within the
process.
sub_lists are automatically added to the "known" list when calling
event_hdl_sub_list_init(), and are removed from the list with
event_hdl_sub_list_destroy().
This allows us to implement a global thread-safe event_hdl deinit()
function that is automatically called on soft-stop thanks to signal(0).
When event_hdl deinit() is initiated, we simply iterate against the known
subscription lists to destroy them.
event_hdl_subscribe_ptr() was slightly modified to make sure that a sub_list
may not accept new subscriptions once it is destroyed (removed from the
known list)
This can occur between the time the soft-stop is initiated (signal(0)) and
haproxy actually enters in the deinit() function (once tasks are either
finished or aborted and other threads already joined).
It is safe to destroy() the subscription list multiple times as long
as the pointer is still valid (ie: first on soft-stop when handling
the '0' signal, then from regular deinit() path): the function does
nothing if the subscription list is already removed.
We partially reverted "BUG/MINOR: event_hdl: make event_hdl_subscribe thread-safe"
since we can use parent mt_list locking instead of a dedicated lock to make
the check gainst duplicate subscription ID.
(insert_lock is not useful anymore)
The check in itself is not changed, only the locking method.
sizeof(event_hdl_sub_list) slightly increases: from 24 bits to 32bits due
to the additional mt_list struct within it.
With that said, having thread-safe list to store known subscription lists
is a good thing: it could help to implement additional management
logic for subcription lists and could be useful to add some stats or
debugging tools in the future.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
event_hdl_sub_list_init() and event_hdl_sub_list_destroy() don't expect
to be called with a NULL argument (to use global subscription list
implicitly), simply because the global subscription list init and
destroy is internally managed.
Adding BUG_ON() to detect such invalid usages, and updating some comments
to prevent confusion around these functions.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
List insertion in event_hdl_subscribe() was not thread-safe when dealing
with unique identifiers. Indeed, in this case the list insertion is
conditional (we check for a duplicate, then we insert). And while we're
using mt lists for this, the whole operation is not atomic: there is a
race between the check and the insertion.
This could lead to the same ID being registered multiple times with
concurrent calls to event_hdl_subscribe() on the same ID.
To fix this, we add 'insert_lock' dedicated lock in the subscription
list struct. The lock's cost is nearly 0 since it is only used when
registering identified subscriptions and the lock window is very short:
we only guard the duplicate check and the list insertion to make the
conditional insertion "atomic" within a given subscription list.
This is the only place where we need the lock: as soon as the item is
properly inserted we're out of trouble because all other operations on
the list are already thread-safe thanks to mt lists.
A new lock hint is introduced: LOCK_EHDL which is dedicated to event_hdl
The patch may seem quite large since we had to rework the logic around
the subscribe function and switch from simple mt_list to a dedicated
struct wrapping both the mt_list and the insert_lock for the
event_hdl_sub_list type.
(sizeof(event_hdl_sub_list) is now 24 instead of 16)
However, all the changes are internal: we don't break the API.
If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.
rid is stored as a uint32_t within struct server, but it was stored as
a signed int within the server event data struct.
Switching from signed int to uint32_t in event_hdl_cb_data_server struct
to make sure it won't overflow.
If 129ecf441 ("MINOR: server/event_hdl: add support for SERVER_ADD and SERVER_DEL events")
is being backported, then this commit should be backported with it.
When building html documentation from doc/lua-api/index.rst, sphinx
complains about some unexpected indentations:
"doc/lua-api/index.rst:3221: WARNING: Unexpected indentation"
Silencing them without altering the original output format.
When building html documentation from doc/lua-api/index.rst, sphinx
complains about some literal blocks ending without a blank line:
"doc/lua-api/index.rst:534: WARNING: Literal block ends without a blank line; unexpected unindent."
Adding the missing blank lines to make sphinx happy
core.register_task(function) may now take up to 4 additional arguments
that will be passed as-is to the task function.
This could be convenient to spawn sub-tasks from existing functions
supporting core.register_task() without the need to use global
variables to pass some context to the newly created task function.
The new prototype is:
core.register_task(function[, arg1[, arg2[, ...[, arg4]]]])
Implementation remains backward-compatible with existing scripts.
Server revision ID was recently added to haproxy with 61e3894
("MINOR: server: add srv->rid (revision id) value")
Let's add it to the hlua server class.
Main lua lock is used at various places in the code.
Most of the time it is used from unprotected lua environments,
in which case the locking is mandatory.
But there are some cases where the lock is attempted from protected
lua environments, meaning that lock is already owned by the current
thread. Thus new locking attempt should be skipped to prevent any
deadlocks from occuring.
To address this, "already_safe" lock hint was implemented in
hlua_ctx_init() function with commit bf90ce1
("BUG/MEDIUM: lua: dead lock when Lua tasks are trigerred")
But this approach is not very safe, for 2 reasons:
First reason is that there are still some code paths that could lead
to deadlocks.
For instance, in register_task(), hlua_ctx_init() is called with
already_safe set to 1 to prevent deadlock from occuring.
But in case of task init failure, hlua_ctx_destroy() will be called
from the same environment (protected environment), and hlua_ctx_destroy()
does not offer the already_safe lock hint.. resulting in a deadlock.
Second reason is that already_safe hint is used to completely skip
SET_LJMP macros (which manipulates the lock internally), resulting
in some logics in the function being unprotected from lua aborts in
case of unexpected errors when manipulating the lua stack (the lock
does not protect against longjmps)
Instead of leaving the locking responsibility to the caller, which is
quite error prone since we must find out ourselves if we are or not in
a protected environment (and is not robust against code re-use),
we move the deadlock protection logic directly in hlua_lock() function.
Thanks to a thread-local lock hint, we can easily guess if the current
thread already owns the main lua lock, in which case the locking attempt
is skipped. The thread-local lock hint is implemented as a counter so
that the lock is properly dropped when the counter reaches 0.
(to match actual lock() and unlock() calls)
This commit depends on "MINOR: hlua: simplify lua locking"
It may be backported to every stable versions.
[prior to 2.5 lua filter API did not exist, filter-related parts
should be skipped]
The check on lua state==0 to know whether locking is required or not can
be performed in a locking wrapper to simplify things a bit and prevent
implementation errors.
Locking from hlua context should now be performed via hlua_lock(L) and
unlocking via hlua_unlock(L)
Using hlua_pushref() everywhere temporary lua objects are involved.
(ie: hlua_checkfunction(), hlua_checktable...)
Those references are expected to be cleared using hlua_unref() when
they are no longer used.
Using hlua_ref() everywhere temporary lua objects are involved.
Those references are expected to be cleared using hlua_unref()
when they are no longer used.
Several error paths were leaking function or table references.
(Obtained through hlua_checkfunction() and hlua_checktable() functions)
Now we properly release the references thanks to hlua_unref() in
such cases.
This commit depends on "MINOR: hlua: add simple hlua reference handling API"
This could be backported in every stable versions although it is not
mandatory as such leaks only occur on rare error/warn paths.
[prior to 2.5 lua filter API did not exist, the hlua_register_filter()
part should be skipped]
hlua init function references were not released during
hlua_post_init_state().
Hopefully, this function is only used during startup so the resulting
leak is not a big deal.
Since each init lua function runs precisely once, it is safe to release
the ref as soon as the function is restored on the stack.
This could be backported to every stable versions.
Please note that this commit depends on "MINOR: hlua: add simple hlua reference handling API"
In core.register_task(): we take a reference to the function passed as
argument in order to push it in the new coroutine substack.
However, once pushed in the substack: the reference is not useful
anymore and should be cleared.
Currently, this is not the case in hlua_register_task().
Explicitly dropping the reference once the function is pushed to the
coroutine's stack to prevent any reference leak (which could contribute
to resource shortage)
This may be backported to every stable versions.
Please note that this commit depends on "MINOR: hlua: add simple hlua reference handling API"
