sanitizer thread CI #301
sanitizer thread CI #301
Conversation
sundb
force-pushed
the
sanitizer_thread_ci
branch
from
acc7681
to
19d291b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Makefile (1 hunks)
Additional comments: 1
src/Makefile (1)
- 113-113: The change to set
MALLOC=libcwhen using the thread sanitizer is appropriate and aligns with best practices to avoid potential conflicts with custom allocators. However, adding a brief comment explaining whylibcis chosen over other allocators when using the thread sanitizer could improve maintainability and clarity for future contributors.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- src/module.c (1 hunks)
- tests/integration/rdb.tcl (2 hunks)
- tests/integration/replication.tcl (1 hunks)
- tests/unit/moduleapi/crash.tcl (1 hunks)
- tests/unit/moduleapi/testrdb.tcl (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- tests/integration/rdb.tcl
- tests/integration/replication.tcl
- tests/unit/moduleapi/crash.tcl
- tests/unit/moduleapi/testrdb.tcl
Additional comments: 1
src/module.c (1)
- 2448-2448: Replacing
sched_yield()withusleep(0)changes the way the current thread yields execution. Whilesched_yield()directly yields to another thread,usleep(0)might introduce a minimal delay, potentially affecting performance. This change, however, ensures compatibility across different platforms and might be more effective in certain scenarios wheresched_yield()is not yielding as expected. It's important to test this change under various load conditions to ensure it does not introduce performance regressions or alter the expected behavior significantly.Consider adding a comment explaining the rationale behind using
usleep(0)oversched_yield(), especially regarding its effectiveness in yielding execution across different platforms.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
WalkthroughThe series of changes span across workflow adjustments, makefile configurations, and source code modifications, primarily focusing on enhancing thread management, sanitizer testing, and handling of race conditions. Notably, the transition from address to thread sanitizer testing in the CI workflow, adjustments for thread sanitizer builds, and the introduction of thread detaching in module tests stand out. These modifications aim to improve the robustness and reliability of the software by addressing thread-related issues and optimizing testing practices. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
sundb
force-pushed
the
sanitizer_thread_ci
branch
from
ac6e722
to
d8c6b2d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (16)
- .github/workflows/daily.yml (1 hunks)
- src/Makefile (1 hunks)
- src/asan.sup (1 hunks)
- src/module.c (1 hunks)
- src/server.c (2 hunks)
- src/server.h (1 hunks)
- tests/integration/rdb.tcl (2 hunks)
- tests/integration/replication.tcl (3 hunks)
- tests/modules/auth.c (1 hunks)
- tests/modules/blockedclient.c (3 hunks)
- tests/modules/blockonbackground.c (3 hunks)
- tests/modules/postnotifications.c (1 hunks)
- tests/modules/propagate.c (2 hunks)
- tests/modules/usercall.c (1 hunks)
- tests/unit/moduleapi/crash.tcl (1 hunks)
- tests/unit/moduleapi/testrdb.tcl (1 hunks)
Files skipped from review as they are similar to previous changes (15)
- src/Makefile
- src/asan.sup
- src/module.c
- src/server.c
- src/server.h
- tests/integration/rdb.tcl
- tests/integration/replication.tcl
- tests/modules/auth.c
- tests/modules/blockedclient.c
- tests/modules/blockonbackground.c
- tests/modules/postnotifications.c
- tests/modules/propagate.c
- tests/modules/usercall.c
- tests/unit/moduleapi/crash.tcl
- tests/unit/moduleapi/testrdb.tcl
| repository: ${{ env.GITHUB_REPOSITORY }} | ||
| ref: ${{ env.GITHUB_HEAD_REF }} | ||
| - name: make | ||
| run: make SANITIZER=address REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS' | ||
| - name: testprep | ||
| # Work around ASAN issue, see https://github.com/google/sanitizers/issues/1716 | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install tcl8.6 tclx -y | ||
| sudo sysctl vm.mmap_rnd_bits=28 | ||
| - name: test | ||
| if: true && !contains(github.event.inputs.skiptests, 'redis') | ||
| run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}} | ||
| - name: module api test | ||
| if: true && !contains(github.event.inputs.skiptests, 'modules') | ||
| run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}} | ||
| - name: sentinel tests | ||
| if: true && !contains(github.event.inputs.skiptests, 'sentinel') | ||
| run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}} | ||
| - name: cluster tests | ||
| if: true && !contains(github.event.inputs.skiptests, 'cluster') | ||
| run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}} | ||
| - name: unittest | ||
| if: true && !contains(github.event.inputs.skiptests, 'unittest') | ||
| run: ./src/redis-server test all | ||
|
|
||
| test-sanitizer-undefined: | ||
| runs-on: ubuntu-latest | ||
| if: | | ||
| (github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) && | ||
| !contains(github.event.inputs.skipjobs, 'sanitizer') | ||
| timeout-minutes: 14400 | ||
| strategy: | ||
| matrix: | ||
| compiler: [ gcc, clang ] | ||
| env: | ||
| CC: ${{ matrix.compiler }} | ||
| steps: | ||
| - name: prep | ||
| if: github.event_name == 'workflow_dispatch' | ||
| run: | | ||
| echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV | ||
| echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV | ||
| echo "skipjobs: ${{github.event.inputs.skipjobs}}" | ||
| echo "skiptests: ${{github.event.inputs.skiptests}}" | ||
| echo "test_args: ${{github.event.inputs.test_args}}" | ||
| echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" | ||
| - uses: actions/checkout@v3 | ||
| with: | ||
| repository: ${{ env.GITHUB_REPOSITORY }} | ||
| ref: ${{ env.GITHUB_HEAD_REF }} | ||
| - name: make | ||
| run: make SANITIZER=undefined REDIS_CFLAGS='-DREDIS_TEST -Werror' LUA_DEBUG=yes # we (ab)use this flow to also check Lua C API violations | ||
| - name: testprep | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install tcl8.6 tclx -y | ||
| - name: test | ||
| if: true && !contains(github.event.inputs.skiptests, 'redis') | ||
| run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}} | ||
| - name: module api test | ||
| if: true && !contains(github.event.inputs.skiptests, 'modules') | ||
| run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}} | ||
| - name: sentinel tests | ||
| if: true && !contains(github.event.inputs.skiptests, 'sentinel') | ||
| run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}} | ||
| - name: cluster tests | ||
| if: true && !contains(github.event.inputs.skiptests, 'cluster') | ||
| run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}} | ||
| - name: unittest | ||
| if: true && !contains(github.event.inputs.skiptests, 'unittest') | ||
| run: ./src/redis-server test all --accurate | ||
| - name: prep | ||
| if: github.event_name == 'workflow_dispatch' | ||
| run: | | ||
| echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV | ||
| echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV | ||
| echo "skipjobs: ${{github.event.inputs.skipjobs}}" | ||
| echo "skiptests: ${{github.event.inputs.skiptests}}" | ||
| echo "test_args: ${{github.event.inputs.test_args}}" | ||
| echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" | ||
| - uses: actions/checkout@v3 | ||
| with: | ||
| repository: ${{ env.GITHUB_REPOSITORY }} | ||
| ref: ${{ env.GITHUB_HEAD_REF }} | ||
| - name: make | ||
| run: | | ||
| sudo apt-get update | ||
| gcc -v | ||
| make SANITIZER=thread REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS' | ||
| - name: testprep | ||
| run: | | ||
| sudo apt-get install tcl8.6 tclx -y | ||
| sudo sysctl vm.mmap_rnd_bits=28 | ||
| - name: test | ||
| if: true && !contains(github.event.inputs.skiptests, 'redis') | ||
| run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" ./runtest --config io-threads 4 --config io-threads-do-reads yes --accurate --verbose --tags -signal --dump-logs ${{github.event.inputs.test_args}} | ||
| - name: module api test | ||
| if: true && !contains(github.event.inputs.skiptests, 'modules') | ||
| run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" CFLAGS='-Werror' ./runtest-moduleapi --config io-threads 4 --config io-threads-do-reads yes --verbose --tags -signal --dump-logs ${{github.event.inputs.test_args}} |
There was a problem hiding this comment.
The introduction of the test-sanitizer-thread job is a key part of the PR's objective to shift focus towards thread sanitizer testing. This job is well-configured to use specific compilers and set up the environment for thread sanitizer testing. However, there are security concerns related to the direct use of GitHub context variables in run steps, which could potentially allow code injection. To mitigate this risk, it's recommended to use environment variables to pass the GitHub context data to the run scripts. Here's an example fix for one of the steps:
- run: |
- echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV
- echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV
+ env:
+ GITHUB_REPOSITORY: ${{ github.event.inputs.use_repo }}
+ GITHUB_HEAD_REF: ${{ github.event.inputs.use_git_ref }}
+ run: |
+ echo "GITHUB_REPOSITORY=$GITHUB_REPOSITORY" >> $GITHUB_ENV
+ echo "GITHUB_HEAD_REF=$GITHUB_HEAD_REF" >> $GITHUB_ENVThis change should be applied to all instances where GitHub context variables are directly used in run steps throughout the workflow file.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| test-sanitizer-thread: | |
| runs-on: ubuntu-latest | |
| if: | | |
| (github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) && | |
| !contains(github.event.inputs.skipjobs, 'sanitizer') | |
| timeout-minutes: 14400 | |
| strategy: | |
| matrix: | |
| compiler: [ gcc, clang ] | |
| compiler: [ gcc-13, clang-15 ] | |
| env: | |
| CC: ${{ matrix.compiler }} | |
| steps: | |
| - name: prep | |
| if: github.event_name == 'workflow_dispatch' | |
| run: | | |
| echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV | |
| echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV | |
| echo "skipjobs: ${{github.event.inputs.skipjobs}}" | |
| echo "skiptests: ${{github.event.inputs.skiptests}}" | |
| echo "test_args: ${{github.event.inputs.test_args}}" | |
| echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" | |
| - uses: actions/checkout@v3 | |
| with: | |
| repository: ${{ env.GITHUB_REPOSITORY }} | |
| ref: ${{ env.GITHUB_HEAD_REF }} | |
| - name: make | |
| run: make SANITIZER=address REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS' | |
| - name: testprep | |
| # Work around ASAN issue, see https://github.com/google/sanitizers/issues/1716 | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install tcl8.6 tclx -y | |
| sudo sysctl vm.mmap_rnd_bits=28 | |
| - name: test | |
| if: true && !contains(github.event.inputs.skiptests, 'redis') | |
| run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}} | |
| - name: module api test | |
| if: true && !contains(github.event.inputs.skiptests, 'modules') | |
| run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}} | |
| - name: sentinel tests | |
| if: true && !contains(github.event.inputs.skiptests, 'sentinel') | |
| run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}} | |
| - name: cluster tests | |
| if: true && !contains(github.event.inputs.skiptests, 'cluster') | |
| run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}} | |
| - name: unittest | |
| if: true && !contains(github.event.inputs.skiptests, 'unittest') | |
| run: ./src/redis-server test all | |
| test-sanitizer-undefined: | |
| runs-on: ubuntu-latest | |
| if: | | |
| (github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) && | |
| !contains(github.event.inputs.skipjobs, 'sanitizer') | |
| timeout-minutes: 14400 | |
| strategy: | |
| matrix: | |
| compiler: [ gcc, clang ] | |
| env: | |
| CC: ${{ matrix.compiler }} | |
| steps: | |
| - name: prep | |
| if: github.event_name == 'workflow_dispatch' | |
| run: | | |
| echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV | |
| echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV | |
| echo "skipjobs: ${{github.event.inputs.skipjobs}}" | |
| echo "skiptests: ${{github.event.inputs.skiptests}}" | |
| echo "test_args: ${{github.event.inputs.test_args}}" | |
| echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" | |
| - uses: actions/checkout@v3 | |
| with: | |
| repository: ${{ env.GITHUB_REPOSITORY }} | |
| ref: ${{ env.GITHUB_HEAD_REF }} | |
| - name: make | |
| run: make SANITIZER=undefined REDIS_CFLAGS='-DREDIS_TEST -Werror' LUA_DEBUG=yes # we (ab)use this flow to also check Lua C API violations | |
| - name: testprep | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install tcl8.6 tclx -y | |
| - name: test | |
| if: true && !contains(github.event.inputs.skiptests, 'redis') | |
| run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}} | |
| - name: module api test | |
| if: true && !contains(github.event.inputs.skiptests, 'modules') | |
| run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}} | |
| - name: sentinel tests | |
| if: true && !contains(github.event.inputs.skiptests, 'sentinel') | |
| run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}} | |
| - name: cluster tests | |
| if: true && !contains(github.event.inputs.skiptests, 'cluster') | |
| run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}} | |
| - name: unittest | |
| if: true && !contains(github.event.inputs.skiptests, 'unittest') | |
| run: ./src/redis-server test all --accurate | |
| - name: prep | |
| if: github.event_name == 'workflow_dispatch' | |
| run: | | |
| echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV | |
| echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV | |
| echo "skipjobs: ${{github.event.inputs.skipjobs}}" | |
| echo "skiptests: ${{github.event.inputs.skiptests}}" | |
| echo "test_args: ${{github.event.inputs.test_args}}" | |
| echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" | |
| - uses: actions/checkout@v3 | |
| with: | |
| repository: ${{ env.GITHUB_REPOSITORY }} | |
| ref: ${{ env.GITHUB_HEAD_REF }} | |
| - name: make | |
| run: | | |
| sudo apt-get update | |
| gcc -v | |
| make SANITIZER=thread REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS' | |
| - name: testprep | |
| run: | | |
| sudo apt-get install tcl8.6 tclx -y | |
| sudo sysctl vm.mmap_rnd_bits=28 | |
| - name: test | |
| if: true && !contains(github.event.inputs.skiptests, 'redis') | |
| run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" ./runtest --config io-threads 4 --config io-threads-do-reads yes --accurate --verbose --tags -signal --dump-logs ${{github.event.inputs.test_args}} | |
| - name: module api test | |
| if: true && !contains(github.event.inputs.skiptests, 'modules') | |
| run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" CFLAGS='-Werror' ./runtest-moduleapi --config io-threads 4 --config io-threads-do-reads yes --verbose --tags -signal --dump-logs ${{github.event.inputs.test_args}} | |
| test-sanitizer-thread: | |
| runs-on: ubuntu-latest | |
| if: | | |
| (github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) && | |
| !contains(github.event.inputs.skipjobs, 'sanitizer') | |
| timeout-minutes: 14400 | |
| strategy: | |
| matrix: | |
| compiler: [ gcc-13, clang-15 ] | |
| env: | |
| CC: ${{ matrix.compiler }} | |
| steps: | |
| - name: prep | |
| if: github.event_name == 'workflow_dispatch' | |
| env: | |
| GITHUB_REPOSITORY: ${{ github.event.inputs.use_repo }} | |
| GITHUB_HEAD_REF: ${{ github.event.inputs.use_git_ref }} | |
| run: | | |
| echo "GITHUB_REPOSITORY=$GITHUB_REPOSITORY" >> $GITHUB_ENV | |
| echo "GITHUB_HEAD_REF=$GITHUB_HEAD_REF" >> $GITHUB_ENV | |
| echo "skipjobs: ${{github.event.inputs.skipjobs}}" | |
| echo "skiptests: ${{github.event.inputs.skiptests}}" | |
| echo "test_args: ${{github.event.inputs.test_args}}" | |
| echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}" | |
| - uses: actions/checkout@v3 | |
| with: | |
| repository: ${{ env.GITHUB_REPOSITORY }} | |
| ref: ${{ env.GITHUB_HEAD_REF }} | |
| - name: make | |
| run: | | |
| sudo apt-get update | |
| gcc -v | |
| make SANITIZER=thread REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS' | |
| - name: testprep | |
| run: | | |
| sudo apt-get install tcl8.6 tclx -y | |
| sudo sysctl vm.mmap_rnd_bits=28 | |
| - name: test | |
| if: true && !contains(github.event.inputs.skiptests, 'redis') | |
| run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" ./runtest --config io-threads 4 --config io-threads-do-reads yes --accurate --verbose --tags -signal --dump-logs ${{github.event.inputs.test_args}} | |
| - name: module api test | |
| if: true && !contains(github.event.inputs.skiptests, 'modules') | |
| run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" CFLAGS='-Werror' ./runtest-moduleapi --config io-threads 4 --config io-threads-do-reads yes --verbose --tags -signal --dump-logs ${{github.event.inputs.test_args}} |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (6)
- src/Makefile (1 hunks)
- src/module.c (1 hunks)
- src/server.c (2 hunks)
- src/server.h (1 hunks)
- tests/modules/postnotifications.c (1 hunks)
- tests/modules/propagate.c (2 hunks)
Additional comments not posted (12)
tests/modules/postnotifications.c (1)
173-173: Ensure proper management of resources within the detached thread and verify there are no dependencies on its completion.tests/modules/propagate.c (2)
188-188: Ensure proper management of resources within the detached thread inpropagateTestThreadCommand.
219-219: Ensure proper management of resources within the detached thread inpropagateTestDetachedThreadCommand.src/Makefile (1)
116-116: Correctly settingMALLOC=libcfor thread sanitizer compatibility is important to avoid potential conflicts with custom allocators.src/server.h (6)
1962-1968: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1999]
Ensure all external libraries and dependencies are properly credited and comply with their licenses.
1962-1968: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1999]
Consider adding detailed comments for each struct member, especially for complex structs like
redisServer, to improve maintainability and understandability of the code.
1962-1968: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1999]
Review the use of macros that directly influence program flow or modify control structures, ensuring they're used safely and don't lead to unpredictable behavior.
1962-1968: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1999]
Verify that all functions that deal with network operations, file I/O, or memory management have proper error handling and validation checks to prevent security vulnerabilities.
1962-1968: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1999]
Evaluate the use of data structures in performance-critical paths, considering if more efficient alternatives are available or if current structures can be optimized.
1962-1968: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1999]
Ensure consistency in naming conventions across structs, functions, and macros to improve code readability and maintainability.
src/server.c (1)
1104-1104: The use ofatomicSetfor updatingdaylight_activeis appropriate and follows best practices for thread-safe operations.src/module.c (1)
2427-2427: Consider verifying the behavior ofusleep(0)across different platforms to ensure it meets the intended purpose of yielding CPU control without introducing unnecessary delays.- usleep(0); + // Consider replacing with a more explicit yielding mechanism if usleep(0) introduces delays. + // For example, pthread_yield() or a conditional wait based on platform-specific behavior.Ensure that this change does not introduce performance regressions or unintended side effects, especially in high-concurrency scenarios.
| @@ -109,11 +109,13 @@ void serverLogRaw(int level, const char *msg) { | |||
| int off; | |||
| struct timeval tv; | |||
| int role_char; | |||
| int daylight_active; | |||
There was a problem hiding this comment.
Initialize daylight_active before its use to ensure defined behavior.
int daylight_active = 0;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| int daylight_active; | |
| int daylight_active = 0; |
Summary by CodeRabbit
MALLOC=libcassignment for better memory allocation handling with thread sanitizer.module.cfor improved Global Interpreter Lock handling.daylight_activefield to atomic type for thread safety in server configurations.