django-typer icon indicating copy to clipboard operation
django-typer copied to clipboard

FileBinaryRead: file is already closed

Open danjac opened this issue 11 months ago • 6 comments

Trying to run this command using FileBinaryRead e.g.

./manage.py my_command path_to_file.txt


app = Typer()

@app.command()
def handle(self, file: typer.FileBinaryRead) -> None:
    """Print out file"""
    for chunk in file:
        print(chunk)

ValueError: I/O operation on closed file

In the Typer docs this should work: https://typer.tiangolo.com/tutorial/parameter-types/file/#filebinaryread

danjac avatar Apr 03 '25 08:04 danjac

@danjac Thanks for the bug report!

I suspect this bug exists because django's BaseCommand has a two-phase parse/execute execution pathway and we've stuffed click's single phase pathway into it which results in some redundant work - for example two contexts are constructed. Click's file type closes the file on context destruction.

I really consider this a click design flaw because parse/execute should be independent exposed but chainable processes - but I doubt click is going to change at this point so we probably need to come up with a workaround. I do not want to inject special case code for these types because working equivalent behavior is easy (see below) and the most correct solution would be to avoid the redundant contexts. This will be more difficult to solve behind Django's interface, so I might not be able to get to it for a bit.

In the mean time it's probably best to avoid these types anyway. This implementation is a similar level of verbosity and also fully encapsulates file/open close. The above code puts the responsibility on the caller to close the file when invoked directly as a function (not through the CLI or call_command):

An equivalent example:

app = Typer()

@app.command()
def handle(self, file: Path) -> None:
    """Print out file"""
    for chunk in file.open("rb"):
        print(chunk)

An example that encapsulates handle ownership more fully:

app = Typer()

@app.command()
def handle(self, file: Path) -> None:
    """Print out file"""
    with open(file, "rb") as f:
        for chunk in f:
            print(chunk)

bckohan avatar Apr 03 '25 17:04 bckohan

Blocked by #210

bckohan avatar Apr 03 '25 17:04 bckohan

Unfortunately I also need to be able to pipe streams to the command as well as use file paths, so for example cat some_file.txt | ./manage.py my_command - does not work with Path as argument.

danjac avatar Apr 04 '25 09:04 danjac

@danjac Weirdly the pipe use case actually works. I'll see what I can do.

bckohan avatar Apr 05 '25 16:04 bckohan

@danjac I don't want to rush this fix because it's delicate. In the meantime here's an easy workaround that works for the pipe and argument case:

app = Typer()

@app.command()
def handle(self, file: typer.FileBinaryRead) -> None:
    """Print out file"""
    if file.closed:
        file = open(file.name, "rb")
    for chunk in file:
        print(chunk)

bckohan avatar Apr 05 '25 19:04 bckohan

Makes sense, thanks!

danjac avatar Apr 05 '25 21:04 danjac