Opened 8 years ago

Closed 8 years ago

#2419 enhancement closed fixed (fixed)

t.i.fdesc improvements

Reported by: itamarst Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, exarkun, itamarst Branch:
Author: Launchpad Bug:

Description (last modified by itamarst)

  1. Add writeToFD, and get rid of duplicate implementations
  1. Add tests
  1. Fix issues with EINTR
  1. Fix __all__

Change History (40)

comment:1 Changed 8 years ago by itamarst

  • Description modified (diff)
  • Status changed from new to assigned

comment:2 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to exarkun
  • Status changed from assigned to new

Implemented in fdesc-2419, ready for review.

comment:3 Changed 8 years ago by itamarst

I expect this will also close #1120, but have no Mac to test on.

comment:4 Changed 8 years ago by exarkun

Which test would demonstrate that #1120 has been fixed?

comment:5 Changed 8 years ago by itamarst

test_writeToClosed, or possibly test_writeToInvalid.

comment:6 Changed 8 years ago by itamarst

Or, well, technically I'm assuming the untested fact that PTYProcess is using writeToFD, but I didn't have much luck trying to trigger the edge case warner was talking about it in PTYProcess directly last time I worked on this code.

comment:7 Changed 8 years ago by exarkun

  • Priority changed from normal to highest

comment:8 Changed 8 years ago by itamarst

  • Owner changed from exarkun to glyph

comment:9 Changed 8 years ago by therve

test_writeAndReadLarge fails consistently for me, under Linux or OS X PPC:

[ERROR]: twisted.test.test_fdesc.ReadWriteTestCase.test_writeAndReadLarge

Traceback (most recent call last):
  File "/Users/buildslave/twisted/fdesc-2419/twisted/test/test_fdesc.py", line 75, in test_writeAndReadLarge
    result.append(self.read())
  File "/Users/buildslave/twisted/fdesc-2419/twisted/test/test_fdesc.py", line 48, in read
    return l[0]
exceptions.IndexError: list index out of range

comment:10 Changed 8 years ago by therve

  • Keywords review removed
  • Owner changed from glyph to itamarst

comment:11 Changed 8 years ago by itamarst

What version Linux? Which version Python?

comment:12 Changed 8 years ago by therve

Every python (2.3/2.4/2.5), under Ubuntu edgy.

comment:13 Changed 8 years ago by therve

FYI, readFromFD returns because os.read raises EAGAIN. The following patch resolves the problem:

Index: twisted/test/test_fdesc.py
===================================================================
--- twisted/test/test_fdesc.py  (revision 19631)
+++ twisted/test/test_fdesc.py  (working copy)
@@ -44,7 +44,7 @@
     def read(self):
         l = []
         res = fdesc.readFromFD(self.r, l.append)
-        if res is None:
+        if res is None and l:
             return l[0]
         else:
             return res
@@ -72,7 +72,10 @@
         self.failUnless(written > 0)
         result = []
         while len(result) < written:
-            result.append(self.read())
+            r = self.read()
+            if not r:
+                break
+            result.append(r)
         result = "".join(result)
         self.assertEquals(len(result), written)
         self.assertEquals(orig[:written], result)

comment:14 Changed 8 years ago by therve

OK after writing the previous comment I found the problem, the condition of the while is wrong:

Index: twisted/test/test_fdesc.py
===================================================================
--- twisted/test/test_fdesc.py  (revision 19631)
+++ twisted/test/test_fdesc.py  (working copy)
@@ -71,7 +71,7 @@
         written = self.write(orig)
         self.failUnless(written > 0)
         result = []
-        while len(result) < written:
+        while len("".join(result)) < written:
             result.append(self.read())
         result = "".join(result)
         self.assertEquals(len(result), written)

comment:15 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to therve

That was exarkun's fault :)

comment:16 Changed 8 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner changed from therve to itamarst

Please use the following (new and officially approved :)) format for docstrings:

"""
Doc.
"""

The work in the branch is great, but I feel uncomfortable about the _posixserialport change, as it seems basically untested. Thoughts ?

comment:17 Changed 8 years ago by itamarst

  • Cc exarkun added
  • Status changed from new to assigned

Well, serial wasn't tested before either, and someone complained about EINTR... let's see what exarkun says.

comment:18 Changed 8 years ago by itamarst

exarkun seemed ok with serial change, since it simply getting rid of duplicate code. I'll do the docstrings.

comment:19 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to therve
  • Status changed from assigned to new

Back to you. I'll leave docstrings in process.py to the refactor we'll be doing there.

comment:20 Changed 8 years ago by therve

  • Keywords review removed
  • Owner changed from therve to itamarst

The branch needs to be merged forward.

Regarding test_writeToFull, I'd be tempted to remove it... If we want to keep it, we may do something like that instead:

while result == len(orig):
     result = self.write(orig)

Maybe we could add a ticket for adding serial tests. Or for deprecating it.

comment:21 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to therve

Ready for review in fdesc-2419-2.

comment:22 Changed 8 years ago by therve

  • Keywords review removed
  • Owner changed from therve to itamarst

You got a problem when you created the branch: there's a strange 'trunk' directory within, that makes the merge impossible.

comment:23 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to therve

How about now?

comment:24 Changed 8 years ago by therve

  • Keywords review removed
  • Owner changed from therve to itamarst

That's better :).

I have only one more complain: change the copyright header to 2001-2007 in posixbase, fdesc, and _posixserialport modules.

Then please merge.

comment:25 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to therve

done.

comment:26 Changed 8 years ago by therve

  • Keywords review removed
  • Owner changed from therve to itamarst

That was OK for me.

comment:27 Changed 8 years ago by exarkun

A lot of conch tests which seem to pass reliably in trunk seem to fail intermittently in this branch. Maybe the changes are just making an existing intermitent failure more likely. It'd be good to understand exactly what's going on here before merging, though.

comment:28 Changed 8 years ago by therve

There's a problem with a modification in t.i.process: some modules (like t.c.unix) rely on the ProcessExitedAlready import, so we must keep it there.

comment:29 Changed 8 years ago by itamarst

No, we should fix those modules. twisted.internet.process is an internal implementation module, it should only be imported by low-level reactor code.

comment:30 Changed 8 years ago by itamarst

Should I be fixing this in this branch, or another?

comment:31 Changed 8 years ago by therve

I have nothing against fixing it in that branch.

comment:32 Changed 8 years ago by exarkun

Avoiding gratuitous breakage would be ideal, though. Can we keep process.ProcessExitedAlready? It can be deprecated and we can delete it completely later.

comment:33 Changed 8 years ago by itamarst

  • Keywords review added
  • Owner changed from itamarst to exarkun

2.3 slave is now happy, tests pass locally.

comment:34 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamarst
  • Docstrings missing for test_fdesc.ReadWriteTestCase.{setUp,tearDown,write,read}
  • Bare except in ReadWriteTestCase.tearDown - what exceptions are really expected here?
  • test_writeAndReadLarge, the loop will never terminate if fewer bytes than expected are received
  • Neither the EAGAIN case nor the EINTR case in writeToFD is tested
  • in process.py, raise error.ProcessExitedAlready should be raise error.ProcessExitedAlready().
  • in process.py, ProcessWriter.writeSomeData used to raise for any OSError other than EPIPE or EAGAIN. Now it will never raise, and for any OSError other than EPIPE, EAGAIN, or EINTR, it will return CONNECTION_LOST. I suppose this is probably correct, but it might make for challenging debugging if an unusual error ever comes back from that write(2). Not exactly sure what should be done about this.
  • procmon.py has no test coverage. Strictly speaking, the changes here aren't necessary, since the compatibility name is in place. It seems pretty unlikely that changing this import is going to introduce any new problems, but it still might be a better idea to omit these from this branch, file a new ticket for the incorrect usage, and let someone who cares enough about runner to fix it all the way deal with it. Up to you.
  • It is bothersome that the incorrect name used in twisted/conch/unix.py only resulted in intermitent failures. This suggests that the test coverage for these codepaths is inadequate. There should probably be another ticket for fixing this. (it's also not clear to me how they ever passed, though, since unix.py should never have been importable).

I ran doc/core/examples/mouse.py and it seems to work still. Of course, this doesn't test the write path for the serial code, so it doesn't really say anything about the changes. :)

Refactoring the write logic into one function is definitely an improvement. Thanks for cleaning up docstrings/comments, too.

comment:35 Changed 8 years ago by therve

  • Owner changed from itamarst to therve

comment:36 Changed 8 years ago by therve

  • Cc itamarst added
  • Keywords review added
  • Owner therve deleted

I did points 1-5, 7.

The problem with t.c.unix is that we use this import to check if it's available for the platform: the import problem results mostly to skipped tests. I had a quick look at it and nothing obvious came out...

comment:37 Changed 8 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:38 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to therve
  • Status changed from assigned to new

Some trailing whitespace left in fdesc.py, otherwise looks good. Please merge. :)

comment:39 Changed 8 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [19815]) Merge fdesc-2419-2

Author: itamar
Reviewer: exarkun, therve
Fixes #2419

Improve twisted.internet.fdesc: add writeToFD, add tests, and manage EINTR
errors.

comment:40 Changed 4 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.