[issue13824] argparse.FileType opens a file and never closes it

[Nils Kattenbeck]

Nils Kattenbeck  added the comment:

A good alternative would be to use pathlib.Path. The only downside would be 
that one has to manually handle `-` but besides that it solves most problems.

Still the fact that file descriptors are kept open should be added as a warning 
to the docs

--
nosy: +Nils Kattenbeck

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[sebix]

Change by sebix :


--
nosy: +sebix

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

Hi,

Are you interested to write test cases? They could be useful for the fix.

Thank you

--
nosy: +matrixise

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[paul j3]

paul j3  added the comment:

While I continue to follow argparse issues, I'm not doing development (with my 
own repository, etc).  

I haven't revisited the issue since 2013, and don't know that much more about 
Python file handling.  Occasionally 'FileType' issues come up on StackOverflow, 
and my most common suggestion is accept a string, and open the file yourself.

In writing a solution, keep backward compatibility foremost in mind.  We don't 
want to mess things up for existing code.  And keep FileType simple, consistent 
with Steven's original intent - as a tool for simple input/output scripts.

As for the idea of 

2 - Making the argument namespace itself support context management

someone could experiment with a new Namespace class with the proper enter/exit 
methods.  The use of a custom Namespace class is documented 

https://docs.python.org/3/library/argparse.html#the-namespace-object

So the idea can be implemented and thoroughly tested without altering the 
default Namespace class and its use.  (With one caution, subparsers won't use 
this custom class.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Mitar]

Mitar  added the comment:

>  So it's already possible to do what you describe, simply by doing:

I think there is an edge case here if a stdin/stdout is opened? It would get 
closed at exist from the context, no?

So I suggest that a slight extension of what open otherwise returns is returned 
which make sure context manager applies only to non-stdin/stdout handles.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Rémi Lapeyre]

Rémi Lapeyre  added the comment:

For information, issue 33927 is related. I removed one of the FileType from 
json.tool argument parser to work around this behavior 
(https://github.com/python/cpython/pull/7865/files#diff-e94945dd18482591faf1e294b029a6afR44).

If paul.j3 does not have his patch anymore I volunteer to implement option 1 to 
have a better alternative in the stdlib.

--
nosy: +remi.lapeyre

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Josh Rosenberg]

Josh Rosenberg  added the comment:

Mitar: argparse.FileType's __call__ (which is what "parses" the argument) 
returns whatever open (aka io.open) returns. Basically, post-parsing, there is 
nothing about the result of FileType that distinguishes it from a manually 
opened file (or when '-' is passed, from stdin/stdout). So it's already 
possible to do what you describe, simply by doing:

parser = argparse.ArgumentParser()
parser.add_argument('input', type=argparse.FileType())
arguments = parser.parse_args()

with arguments.input as f:
   assert arguments.input is f

That already "open[s] eagerly as now, but it would be closed at exit from 
context manager."

The original report did not like that you couldn't prevent clobbering of an 
existing file in write mode (no longer true, since you can pass a mode of 'x' 
just like you can with open), and did not like that the result of parsing was 
implicitly opened immediately without being controllable *immediately* via a 
context manager.

The only real solutions to that problem are either:

1. Making FileType (or a replacement) that is lazier in some way.
2. Making the argument namespace itself support context management

The proposal to check validity and return a curried call to open is problematic 
given the TOCTOU issues, but a curried version that performs little or no 
checks up front, but performs argparse style clean exits if the deferred open 
fails would be reasonable.

The alternative would be to integrate more heavily with the argument namespace, 
such that you could write code like:

with parser.parse_args() as arguments:

and in that scenario, the files would be opened immediately and stored on the 
namespace, but either only FileType, or any type result supporting context 
management, would be bulk __enter__-ed and bulk __exit__-ed upon exiting the 
with block.

As is, a user could do most of this with contextlib.ExitStack, but it's more to 
reimplement (and tricky to get right, and would still rely on argparse to clean 
up files 1 through n-1 if opening file n fails for whatever reason before 
parsing completes).

--
nosy: +josh.r

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Mitar]

Mitar  added the comment:

Why not make FileType instance also a context manager, so you could do 
something like:

with arguments.input as f:
   assert arguments.input is f

For backwards compatibility, FileType would open eagerly as now, but it would 
be closed at exit from context manager.

We should just make sure we do not close stdout/stderr.

--
nosy: +mitar

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[paul j3]

paul j3 added the comment:

From http://bugs.python.org/issue14156 
argparse.FileType for '-' doesn't work for a mode of 'rb'

I learned that 'stdin/out' can be embedded in an 'open' by using the 'fileno()' 
and 'closefd=False' (so it isn't closed at the end of open).

With this, the dummy file context that I implemented in the previous patch, 
could be replaced with:

partial(open, sys.stdin.fileno(), mode=self._mode,..., closefd=False)

However, as Steven Bethard wrote in the earlier issue, setting up tests when 
stdin/out will be redirected is not a trivial problem.  So I don't yet have a 
testable patch.

---
And just for the record, the 'osaccess' testing that I wrote earlier, probably 
should also test that proposed output file string is not already a directory.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[paul j3]

paul j3 added the comment:

An alternative way to delay the file opening, is to return an instance that has 
a `filename` attribute, and has an `open` method.  This can be compactly added 
to the `FileContext` that I defined in the previous patch.  The `FileContext` 
provides the `_ostest` function (testing using os.access), and wrapper for 
stdin/out.


class FileOpener(argparse.FileContext):
# delayed FileType; alt to use of partial()
# sample use:
# with args.input.open() as f: f.read()
def __call__(self, string):
string = self._ostest(string)
self.filename = string
return self
def open(self):
return self.__delay_call__(self.filename)()
file =  property(open, None, None, 'open file property')

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[paul j3]

paul j3 added the comment:

In this patch I implement a FileContext class.  It differs from FileType
in 2 key areas:

- it returns a 'partial(open, filename, ...)'
- it wraps '-' (stdin/out) in a dummy context protecting the file from closure.

The resulting argument is meant to be used as:

with args.input() as f:
f.read()
etc

The file is not opened until value is called, and it will be closed after the 
block.  stdin/out can also be used in this context, but without closing.

The signature for this class is the same as for FileType, with one added 
parameter, 'style'. (alternative name suggestions welcomed).

class argparse.FileContext(mode='r', bufsize=-1, encoding=None, errors=None, 
style='delayed')

The default behavior, style='delayed', is as described above.

style='evaluate' immediately calls the partial, returning an opened file.  
This is essentially the same as FileType, except for the stdin/out context 
wrapping.

style='osaccess', adds os.acccess testing to determine whether the 'delayed' 
file can be read or written.  It attempts to catch the same sort of OS errors 
that  FileType would, but without actually opening or creating the file.

Most of the added tests in test_argparse.py copy the FileType tests.  I had to 
make some modifications to the testing framework to handle the
added levels of indirection.

I have not written the documentation changes yet.

A sample use case is:

import argparse, sys
p = argparse.ArgumentParser()
p.add_argument('-d','--delayed', type=argparse.FileContext('r'))
p.add_argument('-e','--evaluated', type=argparse.FileContext('r', 
style='evaluate'))
p.add_argument('-t','--test', dest='delayed', 
type=argparse.FileContext('r', style='osaccess'))
p.add_argument('-o','--output', type=argparse.FileContext('w', 
style='osaccess'), default='-')
p.add_argument('--unused', type=argparse.FileContext('w', 
style='osaccess'),help='unused write file')
args = p.parse_args()

with args.output() as o:
if args.delayed:
with args.delayed() as f:
print(f.read(), file=o)
if args.evaluated:
with args.evaluated as f:
print(f.read(), file=o)
# f and o will be closed if regular files
# but not if stdin/out
# the file referenced by args.unused will not be created

--
keywords: +patch
Added file: http://bugs.python.org/file31852/patch_3.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[paul j3]

Changes by paul j3 ajipa...@gmail.com:


--
nosy: +paul.j3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Steven Bethard]

Steven Bethard steven.beth...@gmail.com added the comment:

So I generally agree that FileType is not what you want for anything but quick 
scripts that can afford to either leave a file open or to close stdin and/or 
stdout. However, quick scripts are an important use case for argparse, so I 
don't think we should get rid of FileType.

What should definitely happen:

* Someone should add some documentation explaining the risks of using FileType 
(e.g. forgetting to close the file, or closing stdin/stdout when you didn't 
mean to).

What could potentially happen if someone really wanted it:

* Someone could create a safe replacement, e.g. a FileOpenerType that returns 
an object with open() and close() methods that do the right things (or whatever 
API makes sense).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

 Additionally, there is no way to prevent FileType from clobbering an existing 
 file
 when used with write mode.
I think this deserves its own feature request: now that Python 3.3 has an 
“exclusive create” mode, argparse.FileType could gain support for it.  Would 
you open that request?

 Given these issues and others,
We have one issue and one missing feature; what are the other issues?

 it seems to me that the usefulness of FileType is outweighed by propensity to 
 encourage bad
 coding.
I think the problem is not as bad as you paint it.  A great number of unclosed 
file handles may cause problem to some OSes, but that’s it.  On the plus side, 
the fact that argparse accepts for its type argument any callable that can 
check and convert a string input is simple, clean and works.  FileType is 
needed.

In packaging/distutils2 for example we have similar functions that return an 
open file object and never close it: the responsibility is at a higher level.  
Other packaging code calling these functions does so in a with statement.  It 
is not evil by nature.

The problem here is that FileType may return stdin or stdout, so we can’t just 
always close the file object (or maybe we can, say using an atexit handler?).

 Perhaps, it would be best if FileType (or some replacement) simply checked 
 that the file exists
But then you’d run into race conditions.  The only sure was to say if a file 
can be opened is to open it.

--
nosy: +eric.araujo
title: argparse.FileType opens a file without excepting resposibility for 
closing it - argparse.FileType opens a file and never closes it

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

s/sure was/sure way/

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13824] argparse.FileType opens a file and never closes it

[David Layton]

David Layton dmlay...@gmail.com added the comment:

Eric,

checked that the file exists
But then you’d run into race conditions.  The only sure was to say if a
file can be opened is to open it.

I think you misunderstand me. I am NOT suggesting that you open and
close the file. I am saying that you should not open it in the first place.
If I cannot open the file at the point in my program where I actually want
to open it, then fine, I can decide, in my own code, what to do.

propensity to encourage bad
 coding.
I think the problem is not as bad as you paint it.  A great number of
unclosed file handles may cause problem to some OSes, but that’s it.  On
the plus side, the fact that argparse accepts for its type argument any
callable that can check and convert a string input is simple, clean and
works.  FileType is needed.

  Causing a problem on some OSes and not others is worse than causing a
problem on all OSes as it increases the likelihood of buggy code passing
tests and moving to production.
 I think argparse is wonderful; I just think that by having FileType not
open the file, the number of it's use cases is increased. As it stands now,
I would prefer the just pass the argument as a string argument and handling
the opening myself unless:

   1. I wanted my program to open the file at the very beginning of the
   program (where one traditionally handles arg parsing)
   2. I wanted to exit on the first, and only, attempt to open the file
   3. I really did not care if the file closed properly--which, granted, is
   often the case with tiny scripts

The moment any of these is not true due to a change in requirements, I will
have to refactor my code to use a filename arg. Where as if I start out
with a bog-standard filename and open it myself, I can easily add the
behaviour I want. I just don't see FileType as a big convenience. However,
I do see that changing this would break backwards compatibility and would
not want to see that happen. Perhaps a new FileNameType that does some
basic, perhaps, optional checks would have wider use-cases.

I hope this helps.

David Layton

On Fri, Feb 3, 2012 at 2:37 PM, Éric Araujo rep...@bugs.python.org wrote:


 Éric Araujo mer...@netwok.org added the comment:

 s/sure was/sure way/

 --

 ___
 Python tracker rep...@bugs.python.org
 http://bugs.python.org/issue13824
 ___


--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue13824
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com