Skip to content

fix: allow async SMB read responses#167

Merged
afiffon merged 1 commit intoafiffon:mainfrom
suyanhanx:fix/allow-async-read-response
Apr 13, 2026
Merged

fix: allow async SMB read responses#167
afiffon merged 1 commit intoafiffon:mainfrom
suyanhanx:fix/allow-async-read-response

Conversation

@suyanhanx
Copy link
Copy Markdown
Contributor

@suyanhanx suyanhanx commented Apr 9, 2026

Closes #165

Summary

  • allow File::read_block to accept async SMB2 responses
  • reuse the existing pending-response handling instead of rejecting the read immediately

Why

This fixes a real downstream problem in OpenDAL's new SMB backend draft PR: apache/opendal#7374.

That OpenDAL branch is intentionally not carrying a local smb-rs fork. With crates.io smb = 0.11.1, a real Samba behavior run still hits read-path failures like:

IO Error: Invalid argument: Async command is not allowed in this context.

OpenDAL only passes today because its retry layer reconnects and retries those reads. In other words, the downstream behavior suite is green, but only because retries are masking this smb-rs bug.

This change makes read_block consistent with the existing write path, which already calls with_allow_async(true).

Public downstream reproduction

The downstream reproduction is public now:

  • OpenDAL draft PR: apache/opendal#7374
  • OpenDAL commit: 41faefd11f0f76253136e6aaa610bf360342d0a2

To reproduce the bug without this fix:

git clone https://github.com/apache/opendal.git
cd opendal
git fetch https://github.com/suyanhanx/opendal.git feat/smb-service
git checkout 41faefd11f0f76253136e6aaa610bf360342d0a2
cd core
RUST_LOG=debug \
OPENDAL_TEST=smb \
OPENDAL_SMB_ENDPOINT=127.0.0.1:1445 \
OPENDAL_SMB_SHARE=MyShare \
OPENDAL_SMB_ROOT=/ \
OPENDAL_SMB_USER=alice \
OPENDAL_SMB_PASSWORD=alipass \
cargo test -p opendal --features tests,services-smb behavior -- --nocapture

Observed downstream behavior with crates.io smb = 0.11.1:

  • final result is still 92 passed; 0 failed
  • debug logs show many async pending responses
  • some large reads also log Async command is not allowed in this context
  • OpenDAL retry logs then show the same read being retried successfully

Representative downstream log lines:

IO Error: Invalid argument: Async command is not allowed in this context.
will retry after 1s because: Unexpected (temporary) at read ... Async command is not allowed in this context.

Validation

  • cargo check -p smb

To validate this change against the public downstream repro above, temporarily point the OpenDAL checkout to this PR branch and rerun the same behavior command. For example:

[patch.crates-io]
smb = { git = "https://github.com/suyanhanx/smb-rs", branch = "fix/allow-async-read-response" }

Expected result with this PR applied:

  • the same OpenDAL behavior command still finishes 92 passed; 0 failed
  • async pending responses are still observed in debug logs
  • the previous Async command is not allowed in this context read failures disappear
  • OpenDAL no longer needs retry to recover those reads

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 9, 2026

Walkthrough

Updated File::read_block to permit async SMB2 responses by changing the receive options from ReceiveOptions::new() to ReceiveOptions::new().with_allow_async(true), enabling proper handling of asynchronous server responses during file read operations.

Changes

Cohort / File(s) Summary
SMB File Read Async Support
crates/smb/src/resource/file.rs
Modified read_block to enable async response handling by adding with_allow_async(true) to ReceiveOptions, allowing the method to transparently handle async SMB2 status responses from servers like TrueNAS SCALE.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A rabbit reads files through async skies,
No longer blocked by server replies,
One tiny flag makes the session flow free,
Async responses now work—hooray! ✨📖

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: allow async SMB read responses' accurately describes the main change: enabling async SMB2 response handling in the read_block method.
Linked Issues check ✅ Passed The PR implements the exact fix suggested in issue #165: enabling allow_async for read_block by adding .with_allow_async(true) to ReceiveOptions, addressing the primary objective.
Out of Scope Changes check ✅ Passed The change is narrowly focused on enabling async handling for read_block only; no unrelated modifications or scope creep detected beyond the stated objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@suyanhanx
Copy link
Copy Markdown
Contributor Author

Opened the downstream OpenDAL draft PR that depends on this fix: apache/opendal#7374

That PR intentionally avoids carrying a local smb-rs fork. With crates.io smb = 0.11.1, OpenDAL's real Samba behavior run still logs Async command is not allowed in this context on some large reads and only passes because the retry layer reconnects and retries. Once this PR lands upstream, I can update the OpenDAL PR notes to point at the released fix instead of the retry-masked dependency bug.

@afiffon afiffon merged commit 3669975 into afiffon:main Apr 13, 2026
1 check passed
afiffon added a commit that referenced this pull request Apr 13, 2026
- update MSRV to 1.89
- release patches of #162 #167
afiffon added a commit that referenced this pull request Apr 13, 2026
- update MSRV to 1.89
- release patches of #162 #167
afiffon added a commit that referenced this pull request Apr 13, 2026
- update MSRV to 1.89
- release patches of #162 #167
afiffon added a commit that referenced this pull request Apr 13, 2026
- update MSRV to 1.89
- release patches of #162 #167
@suyanhanx suyanhanx deleted the fix/allow-async-read-response branch April 14, 2026 03:25
afiffon added a commit that referenced this pull request Apr 22, 2026
- update MSRV to 1.89
- release patches of #162 #167
@afiffon
Copy link
Copy Markdown
Owner

afiffon commented Apr 22, 2026

I finally got to properly release v0.11.2, which rolls out this patch to crates.io!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: read_block / create_file uses allow_async: false, causing session corruption on async SMB2 responses

2 participants