t.i.fdesc improvements

[itamarst]

1. Add writeToFD, and get rid of duplicate implementations

2. Add tests

3. Fix issues with EINTR

4. Fix __all__

[itamarst]

Implemented in fdesc-2419, ready for review.

[itamarst]

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

[exarkun]

Which test would demonstrate that #1120 has been fixed?

[itamarst]

test_writeToClosed, or possibly test_writeToInvalid.

[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.

[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

[itamarst]

What version Linux? Which version Python?

[therve]

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

[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)

[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)

[itamarst]

That was exarkun's fault :)

[therve]

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 ?

[itamarst]

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

[itamarst]

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

[itamarst]

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

[therve]

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.

[itamarst]

Ready for review in fdesc-2419-2.

[therve]

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

[itamarst]

How about now?

[therve]

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.

[itamarst]

done.

[therve]

That was OK for me.

[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.

[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.

[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.

[itamarst]

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

[therve]

I have nothing against fixing it in that branch.

[exarkun]

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

[itamarst]

2.3 slave is now happy, tests pass locally.

[exarkun]

• 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.

[therve]

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...

[exarkun]

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

[therve]

(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.