◐ Shell
reader mode source ↗
Skip to content

bpo-31158: Fix nondeterministic read in test_pty#3808

Merged
vstinner merged 3 commits into
python:masterfrom
diekmann:fix-issue-31158
Oct 2, 2017
Merged

bpo-31158: Fix nondeterministic read in test_pty#3808
vstinner merged 3 commits into
python:masterfrom
diekmann:fix-issue-31158

Conversation

@diekmann

@diekmann diekmann commented Sep 28, 2017

Copy link
Copy Markdown
Contributor

Sometimes, the test suite fails in test_pty.test_basic()

https://bugs.python.org/issue31158

Acks to @Haypo !!

https://bugs.python.org/issue31158

@pitrou

pitrou commented Sep 28, 2017

Copy link
Copy Markdown
Member

Why not use FileIO instead? Something like:

with io.FileIO(master_fd) as master:
    ...
    s1 = master.readline()
    ...

@diekmann

Copy link
Copy Markdown
Contributor Author

Thanks @pitrou for the suggestion. The following also works on my machine:

diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py
index f283e19..ee9b589 100644
--- a/Lib/test/test_pty.py
+++ b/Lib/test/test_pty.py
@@ -10,6 +10,7 @@ import sys
 import select
 import signal
 import socket
+import io
 import unittest
 
 TEST_STRING_1 = b"I wish to buy a fish license.\n"
@@ -92,17 +93,18 @@ class PtyTest(unittest.TestCase):
             # Restore the original flags.
             os.set_blocking(master_fd, blocking)
 
-        debug("Writing to slave_fd")
-        os.write(slave_fd, TEST_STRING_1)
-        s1 = os.read(master_fd, 1024)
-        self.assertEqual(b'I wish to buy a fish license.\n',
-                         normalize_output(s1))
-
-        debug("Writing chunked output")
-        os.write(slave_fd, TEST_STRING_2[:5])
-        os.write(slave_fd, TEST_STRING_2[5:])
-        s2 = os.read(master_fd, 1024)
-        self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2))
+        with io.FileIO(master_fd, mode='rb', closefd=False) as master:
+            debug("Writing to slave_fd")
+            os.write(slave_fd, TEST_STRING_1)
+            s1 = master.readline()
+            self.assertEqual(b'I wish to buy a fish license.\n',
+                             normalize_output(s1))
+
+            debug("Writing chunked output")
+            os.write(slave_fd, TEST_STRING_2[:5])
+            os.write(slave_fd, TEST_STRING_2[5:])
+            s2 = master.readline()
+            self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2))
 
         os.close(slave_fd)
         os.close(master_fd)

Which version is better?

Problem in version 1:

  • Re-implementing readline

Problem in version 2:

  • Depends on the io module, while the pty module usually only uses the raw os.read and os.write calls. A failure could make debugging harder (is it the pty module or a change in the io module?).
  • I don't like that we have both, master and master_fd hanging around

@diekmann

Copy link
Copy Markdown
Contributor Author

I hope we now have the best of both worlds.

Ideally, this commit is fixuped into the previous commit. Since there is
already a comment on github, I won't rebase.
@vstinner vstinner merged commit e6f62f6 into python:master Oct 2, 2017
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @diekmann for the PR, and @Haypo for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@bedevere-bot

Copy link
Copy Markdown

GH-3852 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2017
* bpo-31158: Fix nondeterministic read in test_pty

* Reuse existing readline implementation from io.

Thx to @pitrou

* Updated comment

Ideally, this commit is fixuped into the previous commit. Since there is
already a comment on github, I won't rebase.
(cherry picked from commit e6f62f6)
@vstinner

vstinner commented Oct 2, 2017

Copy link
Copy Markdown
Member

While the test is not perfect, I don't think that we need perfect quality here. I merged the PR to fix this very old bug: test_pty.test_basic() was failing randomly since 1 or 2 years! (well, probably since the test was written)

@vstinner

vstinner commented Oct 2, 2017

Copy link
Copy Markdown
Member

Oh. I should have fixed the commit message. I just hate the UI for merge a PR. I always miss the commit message edit area.

@vstinner

vstinner commented Oct 2, 2017

Copy link
Copy Markdown
Member

I backported the change manually to 2.7: PR #3853.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants