Skip to content

Move input coroutine management variables to TLS #11576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
runderwo wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move input coroutine management variables to TLS #11576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions include/fluent-bit/flb_input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,8 +578,8 @@ struct flb_libco_in_params {
struct flb_coro *coro;
};

extern pthread_key_t libco_in_param_key;
extern struct flb_libco_in_params libco_in_param;
extern pthread_key_t __thread libco_in_param_key;
extern struct flb_libco_in_params __thread libco_in_param;
Comment on lines +581 to +582
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard libco TLS declarations behind FLB_HAVE_C_TLS

CMakeLists.txt explicitly supports builds where compiler TLS is unavailable (or forced off with FLB_POSIX_TLS), but these declarations now unconditionally require __thread. That breaks the non-C-TLS build path by making flb_input.h depend on compiler TLS syntax in all configurations, so affected targets fail at compile time before the pthread-key fallback can be used. Please switch this to the existing FLB_TLS_* abstraction or gate it with #ifdef FLB_HAVE_C_TLS.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

@runderwo runderwo Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cosmo0920 Open to suggestions here.

Copy link
Contributor

@cosmo0920 cosmo0920 Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the existing TLS abstraction or conditionally compile the declarations like other sources do.

Copy link
Contributor Author

@runderwo runderwo Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If conditional compilation is required, I think that means this approach is broken since the write conflict would still exist on any system without TLS. Should we protect the key variable with a mutex instead?

Comment on lines +581 to +582
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the definition in the source file matches the header declaration
rg -n '__thread.*libco_in_param' src/flb_input.c -B1 -A1

# Check how pthread_key_create is called - should be once, not per-coroutine
ast-grep --pattern 'pthread_key_create($_, $_)'

Repository: fluent/fluent-bit

Length of output: 3133

🏁 Script executed:

# Find the function containing line 652 in flb_input.h
head -700 include/fluent-bit/flb_input.h | tail -60 | cat -n

# Check if this is in a per-coroutine or initialization function
rg -n 'flb_input_coro_collect|pthread_key_create' include/fluent-bit/flb_input.h -B5 -A5

# Check how libco_in_param is actually accessed in the code
rg -n 'libco_in_param' src/flb_input.c | head -30

Repository: fluent/fluent-bit

Length of output: 3082

🏁 Script executed:

# Check how libco_in_param_key is used - getspecific/setspecific or direct access
rg -n 'pthread_getspecific|pthread_setspecific|libco_in_param' src/flb_input.c -B2 -A2 | head -60

# Also check include files
rg -n 'libco_in_param' include/fluent-bit/flb_input.h -B2 -A2

Repository: fluent/fluent-bit

Length of output: 1538

Critical: pthread_key_t must not be __thread—this breaks the TLS mechanism.

A pthread_key_t is a global identifier/handle shared by all threads to access their respective thread-local values via pthread_getspecific/pthread_setspecific. Making it __thread means each thread gets its own key variable. Combined with pthread_key_create at line 652 being called inside flb_input_coro_collect() (not at initialization), each thread creates a different key every time a coroutine is collected—completely breaking the shared-key model.

Choose one TLS approach:

  1. Use __thread directly on the params struct (eliminate pthread_key_t entirely), OR
  2. Keep pthread_key_t global (not __thread) and call pthread_key_create exactly once at initialization (not per-coroutine)

Additionally, libco_in_param at line 582 is declared with __thread but never used—the code uses pthread_getspecific(libco_in_param_key) instead. This unused declaration suggests the two TLS approaches are being conflated and should be reconciled.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@include/fluent-bit/flb_input.h` around lines 581 - 582, The pthread TLS usage
is mixed up: do not declare pthread_key_t __thread; pick one approach and
reconcile libco_in_param_key and libco_in_param. Either (A) remove pthread_key_t
entirely and make the params struct truly thread-local by declaring
libco_in_param as __thread and replace pthread_getspecific/pthread_setspecific
usage with direct access to libco_in_param, or (B) keep libco_in_param_key as a
single global pthread_key_t (remove __thread from libco_in_param_key and
libco_in_param), move the pthread_key_create call out of flb_input_coro_collect
to a one-time initialization path, and ensure all accesses use
pthread_getspecific/pthread_setspecific with libco_in_param_key; also remove the
unused libco_in_param declaration when using option B.

void flb_input_coro_prepare_destroy(struct flb_input_coro *input_coro);

static FLB_INLINE void input_params_set(struct flb_coro *coro,
Expand Down Expand Up @@ -614,14 +614,8 @@ static FLB_INLINE void input_pre_cb_collect(void)
struct flb_libco_in_params *params;

params = pthread_getspecific(libco_in_param_key);
if (params == NULL) {
params = flb_calloc(1, sizeof(struct flb_libco_in_params));
if (params == NULL) {
flb_errno();
return;
}
pthread_setspecific(libco_in_param_key, params);
}
flb_bug(params == NULL);

Comment on lines +617 to +618
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

nl -ba include/fluent-bit/flb_input.h | sed -n '585,670p'

Repository: fluent/fluent-bit

Length of output: 103

🏁 Script executed:

#!/bin/bash
# Read lines 585-670 from flb_input.h
cat -n include/fluent-bit/flb_input.h | sed -n '585,670p'

Repository: fluent/fluent-bit

Length of output: 3099

🏁 Script executed:

#!/bin/bash
# Check the extern declarations and __thread usage
head -100 include/fluent-bit/flb_input.h | cat -n

Repository: fluent/fluent-bit

Length of output: 4123

🏁 Script executed:

#!/bin/bash
# Search for libco_in_param_key declaration
rg "libco_in_param_key" include/fluent-bit/flb_input.h -B 2 -A 2

Repository: fluent/fluent-bit

Length of output: 908

🏁 Script executed:

#!/bin/bash
# Search for libco_in_param_key definition in the source files
rg "libco_in_param_key" src/ --type c -B 1 -A 1 | head -50

Repository: fluent/fluent-bit

Length of output: 204

Eliminate the unsafe assertion by propagating allocation/setup errors.

The assertion at line 617 assumes params is set, but input_params_set() returns void and silently fails if flb_calloc() or pthread_setspecific() fails. When the coroutine resumes and calls input_pre_cb_collect(), the assertion aborts instead of failing cleanly. Additionally, the __thread declaration of pthread_key_t is incorrect—pthread keys are global identifiers, not thread-local variables—and pthread_key_create() should not be called per-coroutine.

Required changes:

  1. Make input_params_set() return an error code (e.g., int with 0 for success)
  2. Check the return value in flb_input_coro_collect() and unwind coroutine creation on failure
  3. Replace the assertion in input_pre_cb_collect() with proper error handling or a fatal log if params is NULL (after confirming bootstrap succeeded)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@include/fluent-bit/flb_input.h` around lines 617 - 618, Change
input_params_set() to return an int error code (0 on success, non-zero on
failure) and propagate allocation/pthread_setspecific errors from flb_calloc and
pthread_setspecific instead of silently swallowing them; update callers (notably
flb_input_coro_collect()) to check the return value and on failure
unwind/cleanup coroutine creation and any allocated resources. Remove the unsafe
flb_bug(params == NULL) assertion in input_pre_cb_collect() and replace it with
proper error handling (return an error/fatal log if params is NULL after
successful bootstrap) so coroutine resume won’t abort; also stop treating
pthread_key_t as __thread—ensure pthread_key_create() is done once globally (not
per-coroutine) and use pthread_setspecific/pthread_getspecific for per-coroutine
storage. Ensure references to input_params_set, flb_calloc, pthread_setspecific,
flb_input_coro_collect, input_pre_cb_collect, pthread_key_t, and
pthread_key_create are updated accordingly.

coll = params->coll;
config = params->config;
coro = params->coro;
Expand Down
4 changes: 2 additions & 2 deletions src/flb_input.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@
#include <fluent-bit/flb_chunk_trace.h>
#endif /* FLB_HAVE_CHUNK_TRACE */

struct flb_libco_in_params libco_in_param;
pthread_key_t libco_in_param_key;
struct flb_libco_in_params __thread libco_in_param;
pthread_key_t __thread libco_in_param_key;
Comment on lines +55 to +56
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C3 '\blibco_in_param_key\b|pthread_key_create\s*\(|pthread_key_delete\s*\(|pthread_once\s*\(|pthread_(get|set)specific\s*\(' include/fluent-bit/flb_input.h src/flb_input.c

Repository: fluent/fluent-bit

Length of output: 2526

Do not make the pthread_key_t itself __thread.

pthread_key_t already identifies a thread-specific slot. With this change, each worker owns a different key handle, while Line 652 in include/fluent-bit/flb_input.h still calls pthread_key_create() on every collect. That leaks one process-wide TSD key per collect and never deletes it, so long-running inputs can exhaust PTHREAD_KEYS_MAX and then fall into the unchecked setup / flb_bug(params == NULL) path at line 617. Keep one process-wide key initialized once, or remove the key entirely and use __thread libco_in_param directly.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/flb_input.c` around lines 55 - 56, The pthread_key_t was incorrectly
declared __thread which gives each thread its own key handle and causes
pthread_key_create() (called in the collect path) to leak process-wide TSD keys;
remove the __thread qualifier from libco_in_param_key so there is a single
process-wide key, and change the initialization logic so
pthread_key_create(libco_in_param_key, ...) is performed exactly once (e.g.,
guarded by a static/atomic "initialized" flag in the module init or in the
function that currently creates the key) or alternatively remove the
pthread_key_t usage entirely and use __thread libco_in_param directly; update
any code paths that assume per-collect key creation (the code invoking
pthread_key_create and the flb_bug(params == NULL) error path) to reflect the
single-key or thread-local-only approach.

Copy link
Contributor Author

@runderwo runderwo Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cosmo0920 Also looking for suggestions here. Removing the key entirely and moving the coroutine params to TLS seems to be the most sensible path, since as the review mentioned, the key is leaked anyway (I observed this during debugging).

Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


#define protcmp(a, b) strncasecmp(a, b, strlen(a))

Expand Down
Loading