Eager open/close of a LazyFile for error handling can make fifo reading fail
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 17.7k
- Forks
- 2.3k
- Avg merge
- 1d 30m
- Merged PRs (30d)
- 18
Description
Overview
When a click.File('rb', lazy=True) is used on Linux named pipes (fifos), the results are not guaranteed to be a simple "lazy read".
LazyFile opens and closes the file eagerly to check for errors (I presume permissions and existence, primarily). If too long passes between the open/close and the subsequent open for reading, the fifo contents may be emptied. If the period between the two open calls is short, something I don't quite understand happens and the read will succeed.
Reproducer
Here's a reproducer which waits 1 second before reading:
import time
import click
@click.command
@click.argument("file", type=click.File("rb", lazy=True))
def main(file):
time.sleep(1)
print(file)
print(file.read())
if __name__ == "__main__":
main()
Test this against a fifo on linux created and written with mkfifo foo; echo 'hi' > foo.
The echo+redirect will block, and will fail when the script is run with a broken pipe.
The click command will hang on file.read(), waiting for data to enter the fifo.
However, without the time.sleep(1) call, this example succeeds, leading to inconsistent behavior which can be hard to troubleshoot.
I thought maybe os has special behavior around non-GCed file descriptors, so I tried it with GC disabled, but it still failed. So I don't understand how it works when it works.
Expected behavior
Using a lazy file to read a fifo should work just as well as using a non-lazy file, and should not be subject to this timing-based behavior.
Suggested Fixes
There is no perfect solution here which retains the current behavior except when it would break.
However, given that the use of fifos is much less common than ordinary files, I see two fixes which I think would be acceptable, plus an alternative approach:
- Explicitly check if it's a fifo before doing the open/close (
stat.S_ISFIFO(os.stat(filename).st_mode)) - Allow callers to control LazyFile and opt-out of the eager open/close
- Ensure it's easy to subclass LazyFile and customize, but otherwise do nothing
I tried out (1) and it seems to work. For (2), I don't see a great interface for this, other than maybe allowing parameters to LazyFile or a customization like click.File(lazy=CustomLazyFileClass).
I put (3) on the table because I appear to be the first person hitting this issue. I see no past issues and no SO questions or similar discussions. If this seems like it's "just too niche to worry about", I'm okay with that. In that case, I just want to be able to easily plug in my customizations.
Right now, I have a workaround which has to directly import from click._compat. It's not very long, but it's not as short as I would like.
long workaround
import typing as t
import click
from click._compat import open_stream
...
class _CustomLazyFile(click.utils.LazyFile):
def __init__(
self,
filename: t.Union[str, "os.PathLike[str]"],
mode: str = "r",
encoding: t.Optional[str] = None,
errors: t.Optional[str] = "strict",
atomic: bool = False,
):
self.name: str = os.fspath(filename)
self.mode = mode
self.encoding = encoding
self.errors = errors
self.atomic = atomic
self._f: t.Optional[t.IO[t.Any]]
self.should_close: bool
if self.name == "-":
self._f, self.should_close = open_stream(filename, mode, encoding, errors)
else:
if "r" in mode and not stat.S_ISFIFO(os.stat(filename).st_mode):
# Open and close the file in case we're opening it for
# reading so that we can catch at least some errors in
# some cases early.
open(filename, mode).close()
self._f = None
self.should_close = True
class LazyBinaryReadFile(click.File):
def convert(
self,
value: str,
param: click.Parameter | None,
ctx: click.Context | None,
) -> t.IO[bytes]:
lf = _CustomLazyFile(value, mode="rb")
if ctx is not None:
ctx.call_on_close(lf.close_intelligently)
return t.cast(t.IO[bytes], lf)
So a simpler path to customize that open(filename, mode).close() call, such that I don't need to replicate __init__, would be nifty.
I would be happy to open a PR with an improvement for this, and would most favor (1) as it makes things "just work" with the least fuss for most cases.
Environment
Environment:
- Python version: 3.11
- Click version: 8.1.7
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with click.utils.LazyFile and the click.File("rb", lazy=True) conversion path described in the issue. Reproduce the behavior on Linux with the provided fifo example, then add coverage for delayed reads; done means lazy fifo reads no longer fail or hang because of the eager open/close behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- cli
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100