[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[STINNER Victor]

STINNER Victor added the comment:

"Should this issue be reopened in light of http://bugs.python.org/issue26372 
(Popen.communicate not ignoring BrokenPipeError)?"

I don't like reopen old issues. IMHO the two issues are different enough to 
justify two entries in the bug tracker.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Akira Li]

Akira Li added the comment:

Should this issue be reopened in light of
http://bugs.python.org/issue26372 (Popen.communicate not ignoring
BrokenPipeError)?

If .close() shouldn't raise BrokenPipeError in .communicate() (and it
shouldn't) then it seems logical that .close() shouldn't raise
BrokenPipeError in .__exit__() too (and in other subprocess.Popen
methods that do not return until the child process is dead such as
subprocess.run())

--
nosy: +akira

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[STINNER Victor]

STINNER Victor added the comment:

No consensus was found, I close the issue.

--
resolution:  -> rejected
status: open -> closed

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Martin Panter]

Martin Panter added the comment:

After understanding the Windows test failure in Issue 21619, I am starting to 
believe that code relying on a BrokenPipeError or EPIPE is flawed. It is an 
inherent unavoidable race condition with the receiving end of the pipe, as long 
as another thread or process is involved, which is always the case for the 
subprocess module. Another way of looking at it is that there is no guarantee 
that your data will have been (or will be) received, even if stdin.close() 
succeeds and does not raise EPIPE or similar. This is because piped data is 
buffered by the OS.

So the proposed change wouldn’t be a significant disadvantage, except for code 
that is already flawed. It is analogous to the argument used for ignoring 
EINTR, because depending on it for handling signals is inherently racy.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Your solution is different: I would prefer to also ignore broken pipe errors
> on close(). I'm not sure that close() can raise a BrokenPipeError in
> practice.

Of course all this code should be inside try/except block that ignores a 
BrokenPipeError.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[STINNER Victor]

STINNER Victor added the comment:

> I thought about different solution:

Your solution is different: I would prefer to also ignore broken pipe errors on 
close(). I'm not sure that close() can raise a BrokenPipeError in practice.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> New patch to fix the bug seen by Serhiy.

I thought about different solution:

try:
if input:
self.stdin.write(input)
finally:
self.stdin.close()

But your approach looks working too,

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I don't see a difference between buffered file and Popen object. Both are 
useless 
after closing, both can raise an exception when flush a buffer on closing. Why 
suppress an exception in one case but not in other? I think this question 
needs wider discussion in Python-Dev.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[STINNER Victor]

STINNER Victor added the comment:

New patch to fix the bug seen by Serhiy.

> Anyway, we closed all pipes

Oh, I forgot to explain that TextIOWrapper.close() closes the buffered file 
even if flush() raised an exception. BufferedWriter.close() does the same.

So stdin.close() always closes the text (binary or text, buffered or not). It's 
not more possible to use stdin after stdin.close(), even if stdin.close() 
raised an exception.

--
Added file: http://bugs.python.org/file38313/popen_exit-2.patch

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[STINNER Victor]

STINNER Victor added the comment:

> Do you want to modify IOBase.__exit__ to ignore I/O errors in close()?

Nope. On files, you want to want to know if your data has been fully written. 
For a subprocess, it's different. You only expect best effort.

The BrokenPipeError exception is raised by Python when an OS function fails 
with EPIPE. This exception has the same purpose than the SIGPIPE signal: warn 
the application that it's now useless to send more data, the consumer will 
ignore it. By the way, since Python checks the result of *all* OS functions, 
SIGPIPE is simply ignored. SIGPIPE and EPIPE have the same purpose.

In the subprocess module, if we get BrokenPipeError, it means that the child 
process stopped reading from stdin (closed it or the process already exited).

Popen.communicate() must close stdin, so why would we pass BrokenPipeError to 
the caller? It's useless, we just stop writing and close the pipe.

Since Popen.__exit__() also closes stdin, I use the same rationale: it useless 
to pass BrokenPipeError to the caller. The caller expects that the process 
exited.

Anyway, we closed all pipes, it's not more possible to communicate with the 
child process. The following example displays "is proc stdin closed? True":
---
import subprocess, sys

args = [sys.executable, '-c', 'pass']
proc = subprocess.Popen(args, stdin=subprocess.PIPE)
try:
with proc:
proc.stdin.write(b'x' * (2**20))
except BrokenPipeError:
pass
print("is proc stdin closed?", proc.stdin.closed)
---

See also: "Why does SIGPIPE exist?"
https://stackoverflow.com/questions/8369506/why-does-sigpipe-exist

--
nosy: +neologix

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Do you want to modify IOBase.__exit__ to ignore I/O errors in close()? I think 
such changes should be discussed in Python-Dev.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[Martin Panter]

Martin Panter added the comment:

I left some minor comments on the documentation.

As a side effect of your rearranging of _stdin_write(), I think it would fix 
the bug with communicate() leaking a BrokenPipeError and leaving a zombie when 
there is less than a buffer’s worth of data to send.

--

___
Python tracker 

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

[issue23570] Change "with subprocess.Popen():" (context manager) to ignore broken pipe error

[STINNER Victor]

New submission from STINNER Victor:

The Popen.communicate() method ignores broken pipe error when writing to stdin. 
I propose to modify Popen.__exit__() to do the same in Python 3.5.

Attached patch implements this suggestion and document it. I added this 
paragraph to Popen doc:

"The context manager ignores broken pipe errors when closing the process’s 
stdin: call explicitly proc.stdin.flush() and proc.stdin.close() to get broken 
pipe errors."

So it's still possible to get broken pipe errors if you need them.

Do you know applications or libraries which rely on broken pipe errors and do 
something different than just ignoring them?

I prefer to leave Python 3.4 unchanged to avoid subtle behaviour changes in 
minor Python releases.

See also:

- issue #21619 which modified Popen.__exit__() to call wait() even if 
stdin.close() raised an exception
- issue #19612 which modified communicate() to handle EINVAL on stdin.write() 
on Windows

--
files: popen_exit.patch
keywords: patch
messages: 237115
nosy: haypo, serhiy.storchaka, vadmium
priority: normal
severity: normal
status: open
title: Change "with subprocess.Popen():" (context manager) to ignore broken 
pipe error
versions: Python 3.5
Added file: http://bugs.python.org/file38311/popen_exit.patch

___
Python tracker 

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