Skip to content

Remove deny_blocking check in moduleBlockClient, cleanup code and doc#2215

Merged
enjoy-binbin merged 4 commits into
valkey-io:unstablefrom
enjoy-binbin:cleanup_module
Sep 2, 2025
Merged

Remove deny_blocking check in moduleBlockClient, cleanup code and doc#2215
enjoy-binbin merged 4 commits into
valkey-io:unstablefrom
enjoy-binbin:cleanup_module

Conversation

@enjoy-binbin

Copy link
Copy Markdown
Member

This flag.deny_blocking check causes some code to becomde dead code.
Some of the code below checks islua or ismulti, but they are marked
with flag.deny_blocking and will return early.

In addition, cleanup some documents, some of them are inaccurate,
and restore the code of blocking_auth_cb in tests/modules/auth.c,
this reply should be returned from core. The reply used to from the
core and was changed to from the module in #1819, and now it is from
the core again.

Cleanup some dead code around #1819.

This flag.deny_blocking check causes some code to becomde dead code.
Some of the code below checks islua or ismulti, but they are marked
with flag.deny_blocking and will return early.

In addition, cleanup some documents, some of them are inaccurate,
and restore the code of blocking_auth_cb in tests/modules/auth.c,
this reply should be returned from core. The reply used to from the
core and was changed to from the module in valkey-io#1819, and now it is from
the core again.

Cleanup some dead code around valkey-io#1819.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@codecov

codecov Bot commented Jun 15, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.21%. Comparing base (6774d17) to head (10ca826).
⚠️ Report is 5 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2215      +/-   ##
============================================
- Coverage     72.21%   72.21%   -0.01%     
============================================
  Files           126      126              
  Lines         70635    70636       +1     
============================================
- Hits          51012    51007       -5     
- Misses        19623    19629       +6     
Files with missing lines Coverage Δ
src/module.h 0.00% <ø> (ø)
src/module.c 9.52% <0.00%> (-0.01%) ⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@enjoy-binbin

Copy link
Copy Markdown
Member Author

@zuiderkwast do you want to review this? You have reviewed the related PR and i have left some comment there. I seem to have forgotten why i was looking this issue, but i can recall there was something odd about it.

Comment thread src/module.c Outdated
Comment thread src/module.c
@zuiderkwast zuiderkwast requested a review from rjd15372 August 5, 2025 12:40
Comment thread src/module.c Outdated
Comment thread src/module.c
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Aug 29, 2025

@rjd15372 rjd15372 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm approving the PR but please add the assert that I mentioned in the code before merging the PR.

Comment thread src/module.c
Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin merged commit eebed88 into valkey-io:unstable Sep 2, 2025
61 checks passed
@enjoy-binbin enjoy-binbin deleted the cleanup_module branch September 2, 2025 01:08
rjd15372 pushed a commit to rjd15372/valkey that referenced this pull request Sep 19, 2025
…valkey-io#2215)

This flag.deny_blocking check causes some code to becomde dead code.
Some of the code below checks islua or ismulti, but they are marked
with flag.deny_blocking and will return early.

In addition, cleanup some documents, some of them are inaccurate,
and restore the code of blocking_auth_cb in tests/modules/auth.c,
this reply should be returned from core. The reply used to from the
core and was changed to from the module in valkey-io#1819, and now it is from
the core again.

Cleanup some dead code around valkey-io#1819.

Signed-off-by: Binbin <binloveplay1314@qq.com>
rjd15372 pushed a commit that referenced this pull request Sep 23, 2025
…#2215)

This flag.deny_blocking check causes some code to becomde dead code.
Some of the code below checks islua or ismulti, but they are marked
with flag.deny_blocking and will return early.

In addition, cleanup some documents, some of them are inaccurate,
and restore the code of blocking_auth_cb in tests/modules/auth.c,
this reply should be returned from core. The reply used to from the
core and was changed to from the module in #1819, and now it is from
the core again.

Cleanup some dead code around #1819.

Signed-off-by: Binbin <binloveplay1314@qq.com>
hpatro pushed a commit to hpatro/valkey that referenced this pull request Oct 3, 2025
…valkey-io#2215)

This flag.deny_blocking check causes some code to becomde dead code.
Some of the code below checks islua or ismulti, but they are marked
with flag.deny_blocking and will return early.

In addition, cleanup some documents, some of them are inaccurate,
and restore the code of blocking_auth_cb in tests/modules/auth.c,
this reply should be returned from core. The reply used to from the
core and was changed to from the module in valkey-io#1819, and now it is from
the core again.

Cleanup some dead code around valkey-io#1819.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants