Repository: fsspec/filesystem_spec
Pull request #1426 (2023-11-13 20:44:39)

Fix inconsistent file mode formatting and comparison (`rb+` changed to `r+b`)
Triggered by https://github.com/fsspec/filesystem_spec/issues/1425

The current processing of the file mode string causes the `b` character to always be on the end of the mode e.g. `rb+` becomes `r+b`.

However in some instances (such as in the memory file system), the valid file modes listed are not compatible with this format, for instance `rb+` is listed. This comparison will never take place given how the mode string is processed.

This PR replaces all instances of `rb+` to `r+b` so that this works now.

lobis: @martindurant after starting this PR I realised that there are different places where the mode is specified (not everything goes through the OpenFile.__enter__) (This is why the new test fails). I could normalize the mode everywhere but it's a bit messy. 

Do you agree on the need to normalize (sort) the file mode string? I think it's dangerous to do string comparisons inside the file system against some reference modes ("wb", "rb+", etc.) when these modes can be in different order (but meaning the same thing). Currently I think the only failing case is when using "+" but still I think it should be addressed (memory filesystem cannot be opened with "rb+" or "r+b" because of the reordering taking place).
martindurant: Maybe you can sprinkle `set()` or `sorted()` where exact equality is tried? In most cases, `in` is used instead.

Patch:
diff --git a/fsspec/caching.py b/fsspec/caching.py
index 2812724..8d441fe 100644
--- a/fsspec/caching.py
+++ b/fsspec/caching.py
@@ -111,7 +111,7 @@ class MMapCache(BaseCache):
             fd.write(b"1")
             fd.flush()
         else:
-            fd = open(self.location, "rb+")
+            fd = open(self.location, "r+b")
 
         return mmap.mmap(fd.fileno(), self.size)
 
diff --git a/fsspec/implementations/cached.py b/fsspec/implementations/cached.py
index 89fab07..eef916b 100644
--- a/fsspec/implementations/cached.py
+++ b/fsspec/implementations/cached.py
@@ -771,10 +771,10 @@ class LocalTempFile:
         self.autocommit = autocommit
 
     def __reduce__(self):
-        # always open in rb+ to allow continuing writing at a location
+        # always open in r+b to allow continuing writing at a location
         return (
             LocalTempFile,
-            (self.fs, self.path, self.fn, "rb+", self.autocommit, self.tell()),
+            (self.fs, self.path, self.fn, "r+b", self.autocommit, self.tell()),
         )
 
     def __enter__(self):
diff --git a/fsspec/implementations/http.py b/fsspec/implementations/http.py
index cdd84c5..7b249bd 100644
--- a/fsspec/implementations/http.py
+++ b/fsspec/implementations/http.py
@@ -802,7 +802,7 @@ async def get_range(session, url, start, end, file=None, **kwargs):
     async with r:
         out = await r.read()
     if file:
-        with open(file, "rb+") as f:
+        with open(file, "r+b") as f:
             f.seek(start)
             f.write(out)
     else:
diff --git a/fsspec/implementations/memory.py b/fsspec/implementations/memory.py
index 32daaf0..4022ff1 100644
--- a/fsspec/implementations/memory.py
+++ b/fsspec/implementations/memory.py
@@ -175,7 +175,7 @@ class MemoryFileSystem(AbstractFileSystem):
             parent = self._parent(parent)
             if self.isfile(parent):
                 raise FileExistsError(parent)
-        if mode in ["rb", "ab", "rb+"]:
+        if mode in ["rb", "ab", "r+b"]:
             if path in self.store:
                 f = self.store[path]
                 if mode == "ab":
diff --git a/fsspec/spec.py b/fsspec/spec.py
index dffbeb8..3bd8d3c 100644
--- a/fsspec/spec.py
+++ b/fsspec/spec.py
@@ -110,6 +110,7 @@ class AbstractFileSystem(metaclass=_Cached):
     async_impl = False
     mirror_sync_methods = False
     root_marker = ""  # For some FSs, may require leading '/' or other character
+    transaction_type = Transaction
 
     #: Extra *class attributes* that should be considered when hashing.
     _extra_tokenize_attributes = ()
@@ -236,13 +237,13 @@ class AbstractFileSystem(metaclass=_Cached):
         for the normal and exception cases.
         """
         if self._transaction is None:
-            self._transaction = Transaction(self)
+            self._transaction = self.transaction_type(self)
         return self._transaction
 
     def start_transaction(self):
         """Begin write transaction for deferring files, non-context version"""
         self._intrans = True
-        self._transaction = Transaction(self)
+        self._transaction = self.transaction_type(self)
         return self.transaction
 
     def end_transaction(self):
diff --git a/fsspec/transaction.py b/fsspec/transaction.py
index d0b5dc9..67584d6 100644
--- a/fsspec/transaction.py
+++ b/fsspec/transaction.py
@@ -1,3 +1,6 @@
+from collections import deque
+
+
 class Transaction:
     """Filesystem transaction write context
 
@@ -13,10 +16,11 @@ class Transaction:
         fs: FileSystem instance
         """
         self.fs = fs
-        self.files = []
+        self.files = deque()
 
     def __enter__(self):
         self.start()
+        return self
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         """End transaction and commit, if exit is not due to exception"""
@@ -27,17 +31,17 @@ class Transaction:
 
     def start(self):
         """Start a transaction on this FileSystem"""
-        self.files = []  # clean up after previous failed completions
+        self.files = deque()  # clean up after previous failed completions
         self.fs._intrans = True
 
     def complete(self, commit=True):
         """Finish transaction: commit or discard all deferred files"""
-        for f in self.files:
+        while self.files:
+            f = self.files.popleft()
             if commit:
                 f.commit()
             else:
                 f.discard()
-        self.files = []
         self.fs._intrans = False
 
 


Test patch:
diff --git a/fsspec/implementations/tests/test_memory.py b/fsspec/implementations/tests/test_memory.py
index 05a40b2..038d72e 100644
--- a/fsspec/implementations/tests/test_memory.py
+++ b/fsspec/implementations/tests/test_memory.py
@@ -191,6 +191,15 @@ def test_seekable(m):
     assert f.tell() == 2
 
 
+# https://github.com/fsspec/filesystem_spec/issues/1425
+@pytest.mark.parametrize("mode", ["r", "rb", "w", "wb", "ab", "r+b"])
+def test_open_mode(m, mode):
+    filename = "mode.txt"
+    m.touch(filename)
+    with m.open(filename, mode=mode) as _:
+        pass
+
+
 def test_remove_all(m):
     m.touch("afile")
     m.rm("/", recursive=True)