Skip to content

Fix read-only memory file handles accepting writes - #2194

Open
hansu650 wants to merge 4 commits into
fsspec:masterfrom
hansu650:fix/memory-read-only-handles
Open

hansu650 wants to merge 4 commits into
fsspec:masterfrom
hansu650:fix/memory-read-only-handles

Conversation

@hansu650

@hansu650 hansu650 commented Sep 27, 2026 •

Copy link
Copy Markdown

Fixes #2193. Also addresses the shared-cursor behavior reported in #2199.

Current design (review follow-up, 2026-10-02)

Following @martindurant's request for live read visibility, this replaces the earlier read-snapshot proposal:

  • Every open returns a handle with its own cursor and mode over the same stored MemoryFile buffer. Reads observe subsequent ordinary writes, truncation and wb replacement contents without copying the file on open.
  • Read-only binary/text handles reject write, writelines, truncate and commit; getbuffer() exposes a read-only view of the shared buffer.
  • Append handles start at EOF and append at the current EOF on each write. r+b retains the existing offset-zero behavior (also the behavior of builtin open); w truncates, and seeking beyond EOF before writing zero-fills the gap.
  • The backing buffer serializes handle seek/read/write operations with a reentrant lock. Pickling recreates that lock and preserves buffer sharing between jointly pickled handles.
  • Merged current upstream af73c65, retaining its modification-time fixes and private transactional append buffers. Transactional append/overwrite commit still publishes a replacement; handles opened before that commit retain their original buffer. Text wrappers can retain their normal read-ahead cache until a seek.
  • Preserves existing no-op close behavior and the historical ability to read from writable memory handles. Direct MemoryFile consumers in other backends remain supported.

Validation

Dedicated Windows / CPython 3.13.15 environment:

  • Initial read-only regression selection on original upstream: 17 failed, 7 passed.
  • The first 21 new live-buffer/cursor checks on the old snapshot design after conflict resolution: 20 failed, 1 passed. All now pass.
  • Final memory/shared-backend selection, including text, transactions, pickle and concurrent readers: 258 passed. The same 258 passed against the installed wheel with isolated imports.
  • Broader core/cache/local/directory/archive integration selection: 1,682 passed, 219 skipped, 11 xfailed, 2 failed. Both failures create filenames containing | and raise Windows WinError 123; the exact two failures reproduce on a clean export of current upstream af73c65 (10 sibling cases pass). No existing skips or expectations were relaxed.
  • Every executable line and branch in the new handle wrapper was covered by that integration run.
  • Repository-wide Ruff check/format, strict Sphinx HTML build, clean-export sdist/wheel builds, Twine and dependency consistency checks passed. Build exports use an explicit local development version without Git metadata.
  • Full Docker/FUSE/cloud/downstream tests and other OS/Python combinations have not been run locally. Existing async-test resource/thread warnings are not claimed resolved.

AI assistance: implemented and validated with OpenAI Codex. The earlier snapshot-only design and its claims are superseded by this revision.

Return independent read snapshots and reject mutation methods without changing writable store handles. Add binary/text, buffer, metadata, pickle and write-mode regression coverage.

Fixes fsspec#2193

Assisted-by: OpenAI Codex <noreply@openai.com>
@itzzdev09

Copy link
Copy Markdown
Contributor

Took a careful look at this. The core problem is real and the enforcement surface is well chosen — overriding write/writelines/truncate/getbuffer plus writable() covers the text-wrapper path too, which is the bit that usually gets missed. Two things I'd want resolved before this goes in.

1. The fix is asymmetric — writable handles still alias.

The changelog says read-only opens mean "another open cannot change their mode", but f.mode = mode still mutates the shared store entry, so the same bug survives between two writable handles:

m.pipe('/f', b'abc')
h1 = m.open('/f', 'r+b')
h2 = m.open('/f', 'ab')
h1.mode    # -> 'ab', silently changed by opening h2
h1 is h2   # -> True

Both are writable so nothing raises today, but mode is per-handle state living on a shared object, which is the same class of bug this PR is fixing. Worth either fixing alongside or explicitly scoping out in the description.

2. This changes read visibility semantics, and I don't think it should be decided here.

Snapshotting readers means an in-place write is no longer visible to an already-open reader:

m.pipe('/f', b'AAA')
r = m.open('/f', 'rb')
w = m.open('/f', 'r+b'); w.seek(0); w.write(b'Z'); w.flush()
r.read()
# main:      b'AA'
# this PR:   b'AAA'

Note main returns b'AA' because the writer's seek(0) moves the reader's position — so this PR incidentally fixes a genuine shared-cursor bug, which is arguably the bigger win here and isn't mentioned in the description.

But it also moves MemoryFileSystem toward object-store semantics (readers get an immutable snapshot) and away from local-filesystem semantics (readers observe in-place writes). That's the same unresolved question that has #2171 paused, where the preference for object-store behaviour was raised and the intended semantics were left open. Settling it as a side effect of a bugfix seems like the wrong venue — probably worth @martindurant's call on the intended model first, since it also determines whether #2171's approach is wanted at all.

Things I checked that are fine, so nobody goes chasing them:

  • No per-open copying. CPython shares BytesIO buffers, so getvalue() into a new BytesIO is cheap: ten rb opens of an 8 MiB pipe()-built file allocate nothing extra; a write()-built one materialises the buffer once and shares it after. .size on read-only handles is not measurably slower.
  • wb-rewrite staleness is unchanged from main — that path already replaced the store entry.
  • test_memory.py 79/79, and 768 passed across the wider suite. The test_jupyter failure I saw is pre-existing and fails on main too.

Happy to see this land once the mode aliasing is settled and the semantics question has an answer.

@hansu650
hansu650 marked this pull request as draft September 28, 2026 06:02
@hansu650

Copy link
Copy Markdown
Author

Thanks for the concrete checks. I reproduced both observations independently on this branch and on the original base 6dee6af5c.

  • Multiple writable opens already return the same object upstream. Upstream has no mode attribute; this patch adds one to that shared object, so opening r+b followed by ab leaves the earlier handle reporting ab. Independent writable handles are not solved by this patch.
  • The reader intentionally sees its open-time snapshot, not later writes. Reject opening implied memory directories as files #2171 discusses directory/object-store semantics, so I do not treat that discussion as approval of read snapshots.

Commit a281040 now explicitly documents both limitations in the class documentation and changelog. The PR description is updated, and I have converted it to Draft until the read-visibility contract is agreed. @martindurant, should read-only opens provide open-time snapshots, or independently positioned handles that observe subsequent writes? If live visibility is required, this implementation needs redesign before it is ready.

Revalidation: 216 memory/backend tests passed; Ruff and the Sphinx HTML build with -W passed. These are scope/documentation clarifications, not a fix for writable-handle aliasing.

Assisted by OpenAI Codex under the submitting account's authorization.

@martindurant

Copy link
Copy Markdown
Member

should read-only opens provide open-time snapshots, or independently positioned handles that observe subsequent writes?

My choice is, that updates from other open handles should be immediately visible - they should share the same underlying buffer. The exact behaviour on local filesystems depends on the OS, so we are free to make the choice here. We should follow the normal posix behaviour, where mode "w" truncates and starts writing at position 0, but mode "a" (or "r+" etc) does not truncate and starts at the end. It is OK to write past the end of a BytesIO - it gets zero-filled.

Address maintainer review on live read visibility while preserving transaction isolation and modification timestamps.

Assisted-by: OpenAI Codex
@hansu650
hansu650 marked this pull request as ready for review October 2, 2026 15:18
@hansu650

hansu650 commented Oct 2, 2026

Copy link
Copy Markdown
Author

Thanks @martindurant. Updated in 442ee75: each open now has its own cursor/mode over the same stored buffer, including writable handles. Existing readers observe ordinary writes and truncation immediately, and read-only buffer views remain read-only. The old snapshot design is removed.

I merged af73c65 and retained its modification-time and transactional-append fixes. Transactional append/overwrite still publishes a replacement only at commit, so already-open handles keep the old buffer across that replacement. Added explicit coverage for both commit and rollback.

One mode detail: r+b still starts at offset 0, matching existing upstream and builtin open; append modes start at EOF and write at the current EOF even after a seek. I kept the existing r+b contract rather than moving its initial cursor to EOF.

Final memory/backend suite: 258 passed, also 258 against the installed wheel. Broader integration: 1,682 passed / 219 skipped / 11 xfailed, with the two invalid-Windows-filename failures independently reproduced on clean current upstream. Ruff, docs and clean packaging checks passed. The PR description now records the updated scope and limitations.

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.

memory:// read handles are writable and the write corrupts the stored file

3 participants