[issue26228] pty.spawn hangs on FreeBSD 9.3, 10.x

2017-10-29 Thread Cornelius Diekmann

Change by Cornelius Diekmann <c.diekm...@googlemail.com>:


--
pull_requests: +4136

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-09-28 Thread Cornelius Diekmann

Cornelius Diekmann <c.diekm...@googlemail.com> added the comment:

Yes, I get the AssertionError with the latest version of PR 3802. From the high 
load avg of my system, you can see that the error occurs very rarely and that I 
need to stress my system to trigger it. With PR 3802, the error occurs way less 
frequently than without it. So we are definitely moving into the right 
direction. With PR 3808, I could not trigger any error (on my system).

Changing the conditions to "b'\n' not in s2" should work. But we actually mean 
to read a line, so this should be better reflected in the code. I prefer 
calling a readline() function directly, which is almost self-documenting code.

> Your code calls read(1) in a loop until it gets the newline character b'\n'.
> Is it better to os.read(1024) in a loop until the output string contains 
> b'\n'?
Behavior may be different if there are multiple short lines in the buffer 
(which should not be the case in the unit test, but this may be a problem if 
somebody copies the code and uses it somewhere else).
pitrou in the discussion of PR 3808 suggests the ultimate answer to the 
question: Just use an existing readline function from the library :-)
I added this to PR 3808.

Personal thought: I care about good code in the unit tests because people might 
look at this as reference how to use a module and copy from it. I want 
the tests to be deterministic, which---as long as the tests pass---implies a 
stable CI ;-)

--

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-09-28 Thread Cornelius Diekmann

Change by Cornelius Diekmann <c.diekm...@googlemail.com>:


--
pull_requests: +3792

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-09-28 Thread Cornelius Diekmann

Cornelius Diekmann <c.diekm...@googlemail.com> added the comment:

I also added a pull request which I cannot get to fail on my system. What do 
you think, haypo?

Sorry for the noise!

--

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-09-28 Thread Cornelius Diekmann

Cornelius Diekmann <c.diekm...@googlemail.com> added the comment:

Oh, one more thing, since it is unclear whether read or write is causing the 
error. I replaced
os.write(slave_fd, TEST_STRING_1)
by
pty._writen(slave_fd, TEST_STRING_1)
which makes sure all bytes are written.
After some time, when the load gets high and the machine gets noisy, I get the 
same error:

0:01:09 load avg: 6.17 [597/1] test_pty failed
test test_pty failed -- Traceback (most recent call last):
  File "XXX/cpython/Lib/test/test_pty.py", line 99, in test_basic
normalize_output(s1))
AssertionError: b'I wish to buy a fish license.\n' != b'I wish to buy a fish 
license.'

So the main problem is a nondeterministic read. But I'm only testing on Linux. 
Other systems with different kernel may behave differently. And the 
documentation allows a kernel to make both short reads and writes.

--

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-09-28 Thread Cornelius Diekmann

Cornelius Diekmann <c.diekm...@googlemail.com> added the comment:

>> Does reading the string b'I wish to buy a fish license.\n' not cause a 
>> problem, too?
>
>This string TEST_STRING_1 is used for a single os.write() call, whereas 
>TEST_STRING_2 is splitted and written in two parts with two os.write() calls.
>
>I prefer minimal change, since I don't know well the pty module.

I like the idea of minimal change, too :D

Yet, I think your patch does not solve the core problem of read/write being 
nondeterministic. In theory, a pty is similar to a pipe (with termios 
processing in the middle). So any os.write to a pty fd is nondeterministic and 
may put less bytes into the pty buffer than given to the write call (see the 
return value of os.write, which test_pty.py does not check). Multiple writes 
are buffered by the kernel, until the buffer is full. So the kernel already 
accumulates the chunked writing for us. Usually, this works fine.

Similarly, a os.read may also be nondeterministic, depending on how many bytes 
are ready in the pty buffer. This may have nothing to do with the chunked 
writing of the test_pty.py module because the kernel is doing the read/write 
syscalls and handling the pty buffer.

Here is a PoC: I checked out your code and stressed my system with it. I have 2 
physical and 2 virtual cores and started 8 instances of the test to stress my 
kernel with scheduling, locking of kernel buffers (the pty buffer), and making 
read/write more nondeterministic.

./python -b -m test -j 8 test_pty -m test_basic -F

Here is what I got:
0:00:13 load avg: 3.25 [119/1] test_pty failed
test test_pty failed -- Traceback (most recent call last):
  File "XXX/cpython/Lib/test/test_pty.py", line 99, in test_basic
normalize_output(s1))
AssertionError: b'I wish to buy a fish license.\n' != b'I wish to buy a fish 
license.'

>> Is reading len(expected) bytes the correct behavior for systems where 
>> normalize_output is needed?
>
>Yeah, I looked at this function. normalize_output() can return a string 
>shorter than the input: len(normalize_output(s2)) <= len(s2).
>
>So I think that len(s2) < len(expected) is correct.

Warning, obscure corner cases ahead.
In theory, given read is completely nondeterministic and we are on one of the 
strange systems which need normalize_output, the following could happen:
We have b'For my pet fish, Eric.\r\n' in the pty buffer. We read b'For my pet 
fish, Eric.\r' from the buffer into s2. Now len(s2) == len(expected) but a 
b'\n' is still unread in the buffer. This would make the test fail. I admit, 
this is a corner case, but also an argument that a clean test case may want to 
have a readline function.

--

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-09-28 Thread Cornelius Diekmann

Cornelius Diekmann <c.diekm...@googlemail.com> added the comment:

Regarding PR 3802:
 * Does reading the string b'I wish to buy a fish license.\n' not cause a 
problem, too?
 * Is reading len(expected) bytes the correct behavior for systems where 
normalize_output is needed?


I also fixed these issues in one of my (unmerged) pull requests (bpo 
issue29070). Unfortunately, the patch I propose does way too many things at the 
same time and I don't have time currently to untangle it :( Feel free to cherry 
pick the _os_readline function:
https://github.com/python/cpython/pull/2932/files#diff-6c897ae50dce4ffa73bde779206a2059R60
Don't hesitate to put your name on it, if you take the time to review and 
cherry pick :)

Cheers!

--

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



[issue31158] test_pty: test_basic() fails randomly on Travis CI

2017-08-10 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

I observed the same issue, but the problem occurs mainly when reading data. In 
my proposed patch in issue29070, I use the existing pty._writen() to make sure 
all data is written. As Martin mentioned, reading is a problem. My patch 
proposes _os_timeout_read(), _os_readline(), _os_read_exactly(), 
_os_read_exhaust_exactly() to give fine-grained, deterministic control. Cheeky 
advertisement: Anybody cares to review issue29070 or cherry pick the 
pty._writen() and _os_read* parts? :)

--

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



[issue29070] Integration tests for pty.spawn on Linux and all other platforms

2017-04-15 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Thank you Martin for your comments in the code review tool. I prepared a new 
patch for the code review tool.

The github changelog from patch v4 (Feb 2017) to my HEAD (currently patch v5, 
Apr 2017) is at:

https://github.com/python/cpython/compare/master...diekmann:wip-issue-29070?expand=1

I hope that having several micro commits instead of one huge patch is helpful. 
In the end, it will be squashed anyway. Do you prefer to continue with bpo 
patches or have a github pull request?

--
Added file: http://bugs.python.org/file46804/pty.patch

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



[issue29070] Integration tests for pty.spawn on Linux and all other platforms

2017-02-09 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Uploaded a new version of the patch.

Changelog of this patch (compared to v3):
 * Fixed reliability issue of existing pty tests.
 * pty.fork() should now also work on systems without os.forkpty(). Added code 
to test this backup path of pty.fork().
 * Reverted my flawed changes to pty.py (broken pipe).
 * All new tests which produce output in the pty.spawn()ed child have the 
terminal in a well-defined state w.r.t. termios.
 * Subtle renaming: PtyTest is now PtyBasicTest. The reason is to execute it 
before the integration tests. All tests are independent and the order is 
irrelevant, but if something fails on a platform, it is easier to debug if we 
run the unit tests before we go to the integration tests.
 * Improved, cleaned, documented, and restructured test code. Thank you Xavier 
and Martin for your very helpful feedback :-)

I also added some small comments in the code review tool.

--
Added file: http://bugs.python.org/file46613/pty.patch

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



[issue29070] Integration tests for pty.spawn on Linux and all other platforms

2017-01-09 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Dear Martin, now I understand your intention. I merged my test suite with 
Chris's fix and documented our insights. SmallPtyTests contains regression 
tests for this issue.

While testing, I found a subtle change in behavior introduced by Chris's patch: 
It is no longer possible to get a broken pipe easily. I made a small adjustment 
to preserve this existing behavior:  In the _copy loop, I changed the order in 
which master_fd and STDIN_FILENO are copied to preserve the BrokenPipeError.


I tuned the slow tests, the complete test suite now needs less than 1 second on 
my system. I updated the documentation to note that the module is no longer 
supposed to be Linux-only.


> I realized that PtyWhiteBoxIntegrationTermiosTest is a reasonable test for 
> the termios module, so it is beneficial. Though it may have been simpler to 
> write it using pty.openpty(), avoiding an extra thread that just copies data 
> between other threads.

I agree completely. However, pty.openpty() does not depend on the _copy() loop 
which is modified by Chris's patch. To add higher test coverage for the changes 
we introduce by merging Chris's fix, I decided to stick to pty.spawn().


I tested on Linux 4.4, Linux 3.13, FreeBSD 11, MacOS 10.11.6 and all tests are 
green. If anything goes wrong on the testbot army, here is the fallback 
strategy:
[I don't expect anything to go wrong, but I'm rather prepared than sorry]
If the test_pty does not finish within one minute on a system, please kill it. 
It would be nice to know on which platform it failed, so we can whitelist the 
module in the tests and the documentation for only the supported platforms.

--
Added file: http://bugs.python.org/file46226/pty.patch

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



[issue29070] Integration tests for pty.spawn on Linux and all other platforms

2017-01-06 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Thank you Martin very much for this very helpful review. I updated and 
simplified the tests and implemented your suggestions.

There are three open issues left.

1)
> It looks like you depend on fixing Issue 26228, but the patch there will 
> conflict with your changes. Maybe merge with the other patch, or propose an 
> alternative fix.

I reverted my changes to pty.py, so the patches no longer conflict for this 
file. Currently, my test suite reveals the pty bug on FreeBSD but does not fix 
it. Since I'm not a FreeBSD expert and I don't want to steal Chris' patch 
[issue26228], I'd like to keep those issues separate. In other words, I'm 
proposing only a testsuite which introduces no behavioral changes and uncovers 
a bug.

This also means that this test suite is currently hanging on FreeBSD and OS X. 
The good news is, the patch in issue26228 fixes the problem.

> The documentation currently mentions the code is only tested on Linux, so it 
> would be nice to update that.
Is it okay to keep this hanging around until the pty module is no longer broken 
on FreeBSD?

2)
> Why does the patch slow the tests down so much? Ideally, it is nice to keep 
> the tests as fast as possible.

The test suite does some heavy integration testing. The test in the class 
PtyWhiteBoxIntegrationReadSlaveTest are the slow ones. The test suite reads a 
large amount of data from the spawned child process. The amount of data was 
chosen to be a few kB in size to make sure that we won't lose data of the child 
and that --in case of a bug in the code-- we don't accidentally succeed due to 
race conditions.

Is 1-2 seconds really too slow? We can reduce the amount of data, but increase 
the risk that a future code change introduces a race condition which is not 
covered by the test suite.


3)
About the system bell pretty printing test:
> This seems platform-dependent. Do you really need to test echoing of control
> characters? We only need to test Python, not the operating system.

Including these integration tests has advantages and disadvantages.

Advantages:
 * Integration tests which deliberately depend on the careful interplay of many 
modules make sure that the overall system is healthy.
 * Modules such as termios or terminal-specific parts of os seem not to be 
tested currently. This test suite depends on their correct functionality and is 
thus a high-level test for them. In addition, pty.spawn() is the perfect place 
to test them.
 * The tests test actual terminal features. Without, we could not uncover if 
e.g., a pty master/slave is replaced by a simple pipe.
 * Tests are set up by increasing order of complexity w.r.t. the underlying 
modules they depend on. If something breaks in the future, these tests are 
designed to ease debugging and directly point to the error.
 * They provide some documentation about the magic of terminals and examples.
 * Those tests are fast.

Disadvantages:
 * Control-Character pretty printing only works on Linux.
 * Relies on the (POSIX-compliant) internals of the operating system.

I wanted to include some tests which depend on some terminal-specific features 
to make sure that the complete setup works and is operational. For example, the 
test suite now documents the difference between ptys and pipes and would 
complain if any code change breaks terminal-specific features. I chose control 
character pretty printing in some tests because this is one of the simplest 
features of terminals to test, debug, and verify. In contrast, for example, it 
would be extremely fragile and probably complete overkill to test things such 
as terminal delays. 

My personal feeling says that the advantages outweigh the disadvantages. If you 
disagree, we can remove them :-)

--
Added file: http://bugs.python.org/file46177/pty_tests.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2017-01-06 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

[no status change, this issue currently does NOT need any attention]

To keep issues separate, I just wanted to document a comment about this issue 
mentioned in issue29070. It refers to the _copy loop.

if STDIN_FILENO in rfds:
 data = stdin_read(STDIN_FILENO)
 if not data:
 fds.remove(STDIN_FILENO)
+# Proposal for future behavior change: Signal EOF to
+# slave if STDIN of master is gone. Solves issue29054.
+# os.write(master_fd, b'\x04')
 else:
 _writen(master_fd, data)

> vadmium 2017/01/04 21:50:26
> I suggest leaving this for the other [issue29054, i.e. this] bug. Another 
> option may be to send SIGHUP
> (though I am far from an expert on Unix terminals :).

http://bugs.python.org/review/29070/diff/19626/Lib/pty.py

--

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2017-01-02 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Status change: I proposed a generic test suite for pty.spawn() in issue29070. 
Once we have agreed on the current behavior of pty.spawn() and the test suite 
is merged, I would like to come back to this issue which requests for a change 
in behavior of pty.spawn(). Currently, I would like to document that this issue 
is waiting for issue29070 and this issue doesn't need any attention.

--

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



[issue26228] pty.spawn hangs on FreeBSD 9.3, 10.x

2017-01-02 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

I just tested pty.spawn() on OS X 10.6.8 and FreeBSD 11 and it also hangs. It 
works on Linux.

Your patch solves the problem. I proposed a test suite for pty.spawn() in 
issue29070. The test suite currently only exposes the problem, as suggested by 
Martin.

--
nosy: +Cornelius Diekmann

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



[issue29070] Integration tests for pty.spawn on Linux and all other platforms

2017-01-02 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

I did a larger update to my proposed patch. I read the pty man pages and posix 
standard and designed a test suite which documents the expected behavior of the 
pty module. The test suite is biased towards my Linux system, but I respect the 
posix standard. Tested on Linux, OS X, and FreeBSD.

The patch introduces no behavioral changes to the actual pty.py module! It 
should be easy to merge.

Changes to pty.py: I made _select and _execlp local definitions of the pty 
module to ease mocking them in the test suite. A comment finally explains why 
pty.py defines STDIN_FILENO, STDOUT_FILENO, ... itself.

All tests pass on Linux.
Tests fail on macOS 10.6.8 and FreeBSD 11. The tests just hang. The patch 
suggested in issue26228 solves these issues. With this patch (or even without 
patch on Linux), the test suite needs about 1 to 2 seconds). I did not include 
the patch of issue26228 into the patch proposed here since I promised no change 
in behavior to the pty module. We now have a lot of test cases for the pty 
module and we are now finally aware that something is broken in the module. 
It's better to have failing tests than to have no tests at all :-)

I included the change to the documentation I proposed in issue29054.

The test suite does several things:

* SmallPtyTests mainly tests the _copy loop and documents current behavior. As 
issue issue26228 has shown, this behavior is not what one would expect. When 
adding the patch for the mentioned issue, one also needs to adapt 
test__copy_eof_on_all and test__copy_eof_on_master. These tests will make it 
easier to resolve issue26228 since they provide a clear documentation of the 
current behavior and contribute the missing test cases.

* PtyTest is left unchanged.

* One has probably noticed that the current test_pty.py suite randomly fails. I 
introduced _os_read_exactly and _os_read_only to make all new tests 
deterministic and easy to debug.

* PtyPosixIntegrationTest does a very simple integration test by spawning the 
programs `true' and `false'. Currently, these tests hang on the platforms where 
pty.spawn() is broken and testing cannot continue. As stated above, with the 
patch, everything is fine also on FreeBSD and OS X.

* PtyMockingExecTestBase is a base class where I prepare to replace the actual 
exec-call made by pty.spawn() to only run local python code. This makes it 
possible to test pty.spawn() in a portable, platform-independent manner (where 
platform-independent means posix-compatible).

* PtyWhiteBoxIntegrationPingPong1000Test exchanges a thousand Ping Pong 
Messages between master and the pty.spawn()ed slave.

* PtyWhiteBoxIntegrationReadSlaveTest tests that we read all data from the 
slave. This test is inspired by msg283912 and it shows that my naive patch in 
the beginning of issue29054 would lose data.

* PtyWhiteBoxIntegrationTermiosTest tests "advanced" features of terminals. The 
pty.spawn() function is the ultimate integration test setup for this.


Waiting for a review :-)

--
Added file: http://bugs.python.org/file46115/pty_tests.patch

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



[issue29070] Integration tests for pty.spawn on Linux and all other platforms

2016-12-25 Thread Cornelius Diekmann

New submission from Cornelius Diekmann:

As Martin Panter noted, "it doesn’t look like pty.spawn() is tested at all" 
[issue26228]. So I wrote some very basic integration tests for pty.spawn.

They work perfectly on my Linux machine. Line 4 of the library module under 
test (pty.py) states: "Only tested on Linux." I don't like this comment ;-)

There are two possibilities how to proceed from here:
1) The tests work perfectly on all other platforms: then we can remove this 
comment from pty.py.
2) The tests fail on other platforms: then we can finally document where this 
module is expected to work and where not.

In either case, I need some help by non-Linux users to run theses tests and 
document their success/failure and whether we have a bug on the specific 
platform (i.e. pty.spawn should work but does not) or whether the tests should 
be skipped (i.e. platform does not support ptys at all).

It would be really awesome if some *BSD and Windows users would join the nosy 
list :-)

Happy Holidays!

--
components: Cross-Build, Tests
files: PtyLinuxIntegrtionTest.patch
keywords: patch
messages: 284003
nosy: Alex.Willmer, Cornelius Diekmann, chris.torek, martin.panter
priority: normal
severity: normal
status: open
title: Integration tests for pty.spawn on Linux and all other platforms
type: enhancement
versions: Python 3.7
Added file: http://bugs.python.org/file46035/PtyLinuxIntegrtionTest.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2016-12-24 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Thank you Martin very much. To resolve this issue, I decided to document the 
current behavior and add test cases for it. No change in behavior is 
introduced. This hopefully allows to close this issue. The test cases for the 
current behavior ensure that we can (at some point in the future) add some 
different behavior without breaking backwards compatibility.

Fixed: Observed behavior is now expected+documented behavior. Improved test 
cases.

Happy Holidays!

--
Added file: http://bugs.python.org/file46023/test_pty_and_doc.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2016-12-23 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Make review tool happy by giving it less broken patch format :)
`make patchcheck` is already happy.
Sorry for the noise :(

--
Added file: http://bugs.python.org/file46015/pty_and_tests.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2016-12-23 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Review tool still did not show the test_pty.py file. Sry.

--
Added file: http://bugs.python.org/file46013/pty.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2016-12-23 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

Removed git patch header from pty.patch to make python code review tool happy. 
Sorry, this is my first contribution.

--
Added file: http://bugs.python.org/file46012/pty.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2016-12-23 Thread Cornelius Diekmann

Cornelius Diekmann added the comment:

I wrote a proper patch for the issue of handling EOF in STDIN, including tests. 
My patch is against the github mirror head, but don't worry, the files I touch 
haven't been touched in recent years ;-)

I only tested on Linux. My patch only addresses the issue in this thread. It 
does not include the patch for issue26228. I still recommend to also merge the 
patch for issue26228 (but I don't have a FreeBSD box to test).

--
versions: +Python 3.6, Python 3.7
Added file: http://bugs.python.org/file46010/pty.patch

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



[issue29054] pty.py: pty.spawn hangs after client disconnect over nc (netcat)

2016-12-23 Thread Cornelius Diekmann

New submission from Cornelius Diekmann:

My OS: Debian GNU/Linux 8.6 (jessie)
Python 3.4.2
pty.py from Python-3.5.2/Lib (pty.py is just a tiny, portable python file which 
did not see many changes)


Bug Report

Steps to Reproduce:

I wrote a very simple python remote shell:

#!/usr/bin/env python3
import pty
pty.spawn('/bin/sh')


It can be run in a terminal (call it TermA) with `nc -e 
./myfancypythonremoteshell.py -l -p 6699`
In a different terminal (call it TermB), I connect to my fancy remote shell 
with `nc 127.0.0.1 6699`.
The shell works fine. In TermB, I quit by pressing ctrl+c.

Observed Behavior: In TermA, the nc process, the python process, and the 
spawned /bin/sh still exist. They still occupy TermA.

Expected Behavior: The client in TermB has disconnected, /bin/sh in TermA can 
no longer read anything from stdin and it should close down. Ultimately, in 
TermA, nc should have exited successfully.

Fix: End the _copy loop in pty.py once EOF in STDIN is reached. Everything 
shuts itself down automatically. I included a small patch to demonstrate this 
behavior.

This patch is not meant to go straight into production. I have not verified if 
this behavior somehow breaks other use cases. This bug report is meant to 
document exactly one specific use case and supply exactly one line of code 
change for it.

This issue is related to issue26228. Actually, it is complementary. issue26228 
wants to return if master_fd is EOF, this issue wants to return if stdin is 
EOF. Both behaviors together looks good to me. By the way, I hacked a hacky 
`assert False` into my patch as a placeholder for issue26228's proper handling 
of exec failures at that part of the code.

I suggest to combine the patches of this issue and issue26228.

--
components: Library (Lib)
files: pty.patch
keywords: patch
messages: 283880
nosy: Cornelius Diekmann, martin.panter
priority: normal
severity: normal
status: open
title: pty.py: pty.spawn hangs after client disconnect over nc (netcat)
type: enhancement
versions: Python 3.4, Python 3.5
Added file: http://bugs.python.org/file46006/pty.patch

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