[issue10644] socket loses data, calling send()/sendall() on invalid socket does not report error and returns all bytes as written
New submission from diekmann : Calling send()/sendall() on an invalid socket does not report an error and returns all bytes as written. Thus, all bytes written to the socket are lost and the application is not informed about that and treats the bytes as successfully sent. The bytes given to the socket are silently lost, this cannot be recovered. The attached file includes an example to reproduce this problem. I defined an invalid socket, when the other side of the connection has closed the connection. Steps to reproduce (see attached file for python implementation): 1) Create listening socket 2) let client connect to it 2.1) send something to the client (optional step) 3) Client terminates connection (now the socket on the server side is invalid) 4) Server calls send/sendall <--- No Error here, but everything is lost 5) Server calls send/sendall again (Now we get the required error) -- components: Library (Lib) files: socketbug.py messages: 123534 nosy: diekmann priority: normal severity: normal status: open title: socket loses data, calling send()/sendall() on invalid socket does not report error and returns all bytes as written type: behavior versions: Python 3.1 Added file: http://bugs.python.org/file19969/socketbug.py ___ Python tracker <http://bugs.python.org/issue10644> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10644] socket loses data, calling send()/sendall() on invalid socket does not report error and returns all bytes as written
diekmann added the comment: ubuntu 9.10 with python 3.1.1+ and debian 5.0.6 with Python 3.1.3rc1 I can reproduce the bug on both systems. Maybe it has been fixed in python 3.2? -- resolution: invalid -> status: pending -> open versions: -Python 3.2 ___ Python tracker <http://bugs.python.org/issue10644> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10644] socket loses data, calling send()/sendall() on invalid socket does not report error and returns all bytes as written
diekmann added the comment: The Documentation states: socket.sendall(bytes[, flags])¶ Send data to the socket. The socket must be connected to a remote socket. The optional flags argument has the same meaning as for recv() above. Unlike send(), this method continues to send data from bytes until either all data has been sent or an error occurs. None is returned on success. On error, an exception is raised, and there is no way to determine how much data, if any, was successfully sent. This is not consistent with the results reproduced above, however, the results from above are exactly what should happen. Maybe there should be a remark, that the return value of sendall (and send) may be system dependent. Or a patch which enforces the documented behviour of sendall, regardless of the operating system would be a nice future feature? -- ___ Python tracker <http://bugs.python.org/issue10644> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue10644] socket loses data, calling send()/sendall() on invalid socket does not report error and returns all bytes as written
diekmann added the comment: > How could it work? Before sending the actual data to the socket, send an empty packet to the socket and check if it is still alive. This may be a large performance issue on a lower level (if the connection is TCP, we want to wait for the ACK), but on a higher level, when using sendall() with larger arguments, it may be very convenient and the performance overhead may be barely noticeable. I guess when using high-level functions like sendall(), the bare funcionality is preferred over this performance issue. -- ___ Python tracker <http://bugs.python.org/issue10644> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue31158] test_pty: test_basic() fails randomly on Travis CI
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 <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
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 <http://bugs.python.org/issue29070> ___ ___ 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
Cornelius Diekmann 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 <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
Cornelius Diekmann 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 <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
Cornelius Diekmann 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 <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
Cornelius Diekmann 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 <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
Change by Cornelius Diekmann : -- pull_requests: +3792 ___ Python tracker <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
Cornelius Diekmann 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&paste from it. I want the tests to be deterministic, which---as long as the tests pass---implies a stable CI ;-) -- ___ Python tracker <https://bugs.python.org/issue31158> ___ ___ 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
Change by Cornelius Diekmann : -- pull_requests: +4136 ___ Python tracker <https://bugs.python.org/issue26228> ___ ___ 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)
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
<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)
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 <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)
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 <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)
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 <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)
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 <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)
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 <http://bugs.python.org/issue29054> ___ ___ 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
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 <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
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 <http://bugs.python.org/issue29070> ___ ___ 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
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 <http://bugs.python.org/issue26228> ___ ___ 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)
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 <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)
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 <http://bugs.python.org/issue29054> ___ ___ 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
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 <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
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 <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
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 <http://bugs.python.org/issue29070> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
