thread_storage: Fix incorrectly to detect C TLS existence#12067
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR changes the C TLS feature probe, moves ChangesTLS Initialization Fix and Test Coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/internal/thread_storage.c (1)
13-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding multi-threaded coverage.
This test validates the single-thread get/set/init sequence, but the actual fix addresses
pthread_once-guarded concurrent key initialization across multiple threads. A test spawning several threads that callFLB_TLS_GET/FLB_TLS_SETconcurrently would more directly cover the race this PR fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/thread_storage.c` around lines 13 - 30, The current test_thread_storage_get_before_init only covers a single-thread get/set/init flow and does not exercise the pthread_once-protected concurrent initialization path. Add a new multi-threaded test around thread_storage_ctx that spawns several threads and has them call FLB_TLS_GET and FLB_TLS_SET concurrently, then assert the TLS key is initialized safely and the values remain consistent across threads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/internal/thread_storage.c`:
- Around line 13-30: The current test_thread_storage_get_before_init only covers
a single-thread get/set/init flow and does not exercise the
pthread_once-protected concurrent initialization path. Add a new multi-threaded
test around thread_storage_ctx that spawns several threads and has them call
FLB_TLS_GET and FLB_TLS_SET concurrently, then assert the TLS key is initialized
safely and the values remain consistent across threads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3052c79e-38ee-41bb-aef9-88e8cac11784
📒 Files selected for processing (4)
CMakeLists.txtinclude/fluent-bit/flb_thread_storage.htests/internal/CMakeLists.txttests/internal/thread_storage.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/internal/thread_storage.c`:
- Around line 34-41: The shared condition variable usage in the thread setup
flow can leave the test stuck because the same cond is used for both readiness
and start synchronization. Update the thread coordination in thread_storage.c,
specifically the logic around the worker-side ready increment and the main-side
wait loop, so that the ready notification wakes all waiters or the ready/start
phases use separate condition variables. Keep the existing symbols
thread->ready, thread->start, thread->cond, and the wait/signaling code in sync
so no worker can consume a wakeup intended for the main thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bb20306-237c-43e0-a3b2-ebe93cf7cb93
📒 Files selected for processing (1)
tests/internal/thread_storage.c
4142982 to
71d955b
Compare
|
is there a way we can avoid calling pthread_once() ? since it will be called over and over, maybe fix the root cause where the initialization is missing ? |
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
71d955b to
645d6ed
Compare
Oh! The main fix of this PR is incorrectly C TLS detection. So, I removed the pthread_once usages and tweak initialization order of worker context. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/internal/thread_storage.c (1)
34-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winShared condition variable can still hang the test
The same
condis used for both the "ready" (worker→main) and "start" (main→workers) phases. At line 36,pthread_cond_signalwakes only one waiter. If a previously-created worker (waiting at line 39 onstart == FALSE) consumes the signal instead of main (waiting at line 103 onready < created), the signal is wasted. After the last worker signals,ready == createdbut main may still be asleep inpthread_cond_waitwith no remaining signals to wake it — deadlock.This was previously flagged and marked addressed, but the current rewritten file still uses
pthread_cond_signalat line 36. Changing topthread_cond_broadcastensures main is always woken.🔒 Proposed fix: broadcast instead of signal on worker ready
pthread_mutex_lock(thread->lock); (*thread->ready)++; - pthread_cond_signal(thread->cond); + pthread_cond_broadcast(thread->cond);#!/bin/bash set -euo pipefail # 1. Confirm the current worker-side signal is still pthread_cond_signal (not broadcast) echo "=== Worker-side signal (expect pthread_cond_signal) ===" sed -n '34,41p' tests/internal/thread_storage.c # 2. Confirm main-side wait and broadcast echo "=== Main-side wait/broadcast ===" sed -n '101,107p' tests/internal/thread_storage.c # 3. Verify the test file is registered in the build echo "=== CMakeLists registration ===" rg -n 'thread_storage' tests/internal/CMakeLists.txt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/thread_storage.c` around lines 34 - 41, The worker-to-main ready notification in thread_storage.c still uses pthread_cond_signal, which can wake the wrong waiter and leave main blocked in the ready wait. Update the synchronization in the worker setup path around the pthread_cond_signal call in the thread helper so it uses pthread_cond_broadcast instead, ensuring any main thread waiting on the shared condition variable is always awakened while preserving the existing ready/start handshake in thread->ready and thread->start.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/internal/thread_storage.c`:
- Around line 34-41: The worker-to-main ready notification in thread_storage.c
still uses pthread_cond_signal, which can wake the wrong waiter and leave main
blocked in the ready wait. Update the synchronization in the worker setup path
around the pthread_cond_signal call in the thread helper so it uses
pthread_cond_broadcast instead, ensuring any main thread waiting on the shared
condition variable is always awakened while preserving the existing ready/start
handshake in thread->ready and thread->start.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08eddafa-fcf5-4b7e-8717-beeca469d504
📒 Files selected for processing (4)
CMakeLists.txtsrc/flb_config.ctests/internal/CMakeLists.txttests/internal/thread_storage.c
✅ Files skipped from review due to trivial changes (1)
- tests/internal/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
645d6ed to
debb55e
Compare
We incorrectly used __tls_get_addr in thread storage detection.
Instead, we just use __thread attribute in that detector and ensuring pthread thread storage to be initialized once on each of the threads.
Also, I added a test cases for confirming define/get/set of thread storage.
Closes #12065.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit