Skip to content

Clear Open client state on close-response error#359

Merged
jborean93 merged 2 commits into
jborean93:masterfrom
henrik-nil-acc:pr/open-close-state
Jun 24, 2026
Merged

Clear Open client state on close-response error#359
jborean93 merged 2 commits into
jborean93:masterfrom
henrik-nil-acc:pr/open-close-state

Conversation

@henrik-nil-acc

@henrik-nil-acc henrik-nil-acc commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

When receive() raised anything other than FileClosed, the
old close path propagated the exception with _connected still
True and the file_id still in session.open_table. A
subsequent close() reissued SMB CLOSE on a dead file_id, and
session.disconnect(close=True) walked the stale entry.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.78%. Comparing base (66ee483) to head (9fb8815).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
- Coverage   98.78%   98.78%   -0.01%     
==========================================
  Files          24       24              
  Lines        5289     5288       -1     
==========================================
- Hits         5225     5224       -1     
  Misses         64       64              
Flag Coverage Δ
macOS 69.09% <52.94%> (+0.29%) ⬆️
py3.10 98.78% <100.00%> (-0.01%) ⬇️
py3.11 98.78% <100.00%> (-0.01%) ⬇️
py3.12 98.78% <100.00%> (-0.01%) ⬇️
py3.13 98.78% <100.00%> (-0.01%) ⬇️
py3.14 98.78% <100.00%> (-0.01%) ⬇️
ubuntu 96.70% <100.00%> (-0.01%) ⬇️
windows 98.71% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jborean93 jborean93 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there a real life scenario we can test with rather than doing it with the mocks? I definitely prefer proper integration tests over unit tests that are just testing the shape of the code.

Comment thread src/smbprotocol/open.py Outdated
b"\xff" * 16 carries documented meaning per MS-SMB2
3.2.4.1.4 ChainedFileId. The meaning belongs at the
declaration, not behind a comment at one assignment site.
When receive() raised anything other than FileClosed, the
old close path propagated the exception with _connected still
True and the file_id still in session.open_table. A
subsequent close() reissued SMB CLOSE on a dead file_id, and
session.disconnect(close=True) walked the stale entry.
@henrik-nil-acc

Copy link
Copy Markdown
Contributor Author

Is there a real life scenario we can test with rather than doing it with the mocks? I definitely prefer proper integration tests over unit tests that are just testing the shape of the code.

Not very easy since it's a race condition.

This works against samba:
Point SMB_SERVER/SMB_SHARE/SMB_USER/SMB_PASSWORD at a writable share:

import gc, multiprocessing as mp, os, warnings

SERVER = os.environ["SMB_SERVER"]
SHARE = os.environ.get("SMB_SHARE", "share")
PORT = int(os.environ.get("SMB_PORT", "445"))
USER = os.environ.get("SMB_USER")
PASSWORD = os.environ.get("SMB_PASSWORD")
ROOT = rf"\\{SERVER}\{SHARE}\repro-359"
DIRS, FILES_PER_DIR, PROCESSES = 20, 50, 12


def _populate():
    from smbclient import makedirs, open_file, register_session
    from smbclient.shutil import rmtree
    register_session(SERVER, username=USER, password=PASSWORD, port=PORT)
    rmtree(ROOT, ignore_errors=True, port=PORT)
    makedirs(ROOT, exist_ok=True, port=PORT)
    for d in range(DIRS):
        sub = rf"{ROOT}\dir-{d:04d}"
        makedirs(sub, exist_ok=True, port=PORT)
        for f in range(FILES_PER_DIR):
            with open_file(rf"{sub}\file-{f:04d}.txt", mode="w", port=PORT) as fd:
                fd.write("x")


def _worker(queue):
    gc.set_threshold(1, 1, 1)
    count = 0

    def _show(message, category, *args, **kwargs):
        nonlocal count
        if category is ResourceWarning and "unclosed SMB handle" in str(message):
            count += 1

    warnings.simplefilter("always")
    warnings.showwarning = _show
    from smbclient import register_session
    from smbclient.shutil import rmtree
    register_session(SERVER, username=USER, password=PASSWORD, port=PORT)
    rmtree(ROOT, ignore_errors=True, port=PORT)
    gc.collect()
    queue.put(count)


def main():
    _populate()
    ctx = mp.get_context("spawn")
    queue = ctx.Queue()
    procs = [ctx.Process(target=_worker, args=(queue,)) for _ in range(PROCESSES)]
    for p in procs:
        p.start()
    total = sum(queue.get() for _ in procs)
    for p in procs:
        p.join()
    print(f"unclosed SMB handle warnings: {total}")


if __name__ == "__main__":
    main()
$ PYTHONWARNINGS=always python repro_359.py   # master
unclosed SMB handle warnings: 171
$ PYTHONWARNINGS=always python repro_359.py   # this PR
unclosed SMB handle warnings: 0

@jborean93

Copy link
Copy Markdown
Owner

Not very easy since it's a race condition.

That's fair enough thanks for clarifying.

@jborean93
jborean93 merged commit 3b5b3e9 into jborean93:master Jun 24, 2026
19 checks passed
@henrik-nil-acc
henrik-nil-acc deleted the pr/open-close-state branch June 25, 2026 04:04
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.

2 participants