Willy,
Am 27.02.20 um 03:43 schrieb Willy Tarreau:
> On Wed, Feb 26, 2020 at 04:20:51PM +0100, Tim Duesterhus wrote:
>> This patch replaces the ad-hoc generation of stream's `unique_id` values
>> by calls to `stream_generate_unique_id`.
>
> It seems to me that it won't generate the unique_id anymore if there
> is no unique-id-header directive in the config :
>
>> http_manage_client_side_cookies(s, req);
>>
>> /* add unique-id if "header-unique-id" is specified */
>> + if (sess->fe->header_unique_id &&
>> !LIST_ISEMPTY(&sess->fe->format_unique_id)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> All the unique-id generation seems to be enclosed in this. Am I missing
> something ?
Yes. The `header_unique_id` check is only in `http_process_request`.
I've merged the previous two ifs into a single one. A unique ID in
`http_process_request` is only generated when it is actually sent to the
upstream server. If it is not sent we don't need to generate one,
because no one is seeing it. This might technically be a slight behavior
change (the unique ID is generated later), but the user won't see a
difference.
By replacing all the ad-hoc generation of the unique IDs with a call to
`stream_generate_unique_id` the unique ID will be generated when it is
first used. This is happening in this order:
1. unique-id-header
2. unique-id sample fetch
3. Logging
I specifically added debug code locally that printed a message with the
current function when the unique ID is actually generated for a request.
If neither the header is set, not the sample fetch is used then an ID
will still be generated for the logging.
> (...)
>
> Other than that I have a suggestion, I've seen recently in this thread
> a few calls to strlen() on unique_id and header_unique_id. I think they
> should be turned to ist so that the length is always stored with them
> and we don't need to run strlen on them anymore at runtime. And this
> will simplify the header addition which will basically look like this:
>
> http_add_header(htx, sess->fe->header_unique_id, s->unique_id);
I was thinking about this, but didn't make the change to keep the diff
small and because it increases the size of a stream by an additional
`size_t` which I'm not sure is desired. I can create a follow up patch
if you want.
Best regards
Tim Düsterhus
