Opened 3 years ago

Closed 18 months ago

#7884 enhancement closed fixed (fixed)

twisted.python.sendmsg should work with the Python 3 stdlib implementation

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/sendmsg-py3-7884-2
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description

Python 3 has a standard lib implementation of twisted.python.sendmsg, so we should be able to use it. The ideal would be:

  • Move sendmsg.c to _sendmsg.c
  • Write a wrapper that calls the _sendmsg.c for Python 2.
  • Deprecate use of the wrapper.
  • Write a new API that calls _sendmsg.c on Python 2 and the stdlib implementation on Python 3.

Change History (19)

comment:1 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/sendmsg-py3-7884

(In [44639]) Branching to sendmsg-py3-7884.

comment:2 Changed 3 years ago by hawkowl

Keywords: review added

This ticket adds the new implementation. The old API should probably be deprecated in another ticket, so there's a nice news message. The old API should be deprecated so that there's a consistent interface on Python2+3.

Builders spun, please review.

comment:3 Changed 3 years ago by hawkowl

I've ported the examples and the docs to Python 3, and to use the new 2/3 API.

comment:4 Changed 3 years ago by Moshe Zadka

Keywords: review removed
Owner: set to hawkowl

Hi HawkOwl,

Thanks for working on this, and thanks for noticing Py3 already having something so we don't have to do it ourselves :)

Some issues which should be addressed:

  1. I have concerns about "return b'\n'.join([b] + list(self.args)).decode()". Sure, it's for a process that in theory we control -- but if some weird combination of libc or Python compilation options makes it spew a greeting in, say, Latin-1-encoded French, this will be an exception. Maybe have a "safe" decoding, where we skip undecodable stuff? (Even that scares me, but at least it won't crash, and the programmer has a chance to figure out what happened.)
  2. "class NewSendmsgTests(TestCase):" What happens when we are Python 3-only? Do we rename this to "SendmsgTests"? Or what happens when a betterer sendmsg way comes along? A better name would be "Py3SendmsgTests" or maybe "BuiltinSendmsgTests"
    1. Same comment about "newpullpipe"
  3. In NewSendmsgTests.tearDown, we do "input.close();output.close()". In many tests we do "input.close()". Does Python guarantee double-close is correct? (Either put in an explicit comment if it does, or change this to be safer if it doesn't)
  4. NewSendmsgTests.test_shortsend: "+ self.assertTrue(sent < len(message))"
    1. This test is Py3 only, you could use assertLessThanEqual
    2. If not, please add an explicit message.
    3. If we ever chance upon a UNIX configured for 1MB buffers, we fail the test? (Might seem ludicrous, but this causes me slight discomfort).
  5. In the "spawn" methods (in both test classes), we're overriding PYTHONPATH rather than adding to it, and also completely nuking the rest of the environment. This is a change from what we do now, which would break 3rd parties running the test suite before building a package with a custom environment (for example, to modify how libc works).

Here are some improvements you might want to consider

  1. In newpullpipe, do we really need "/u/b/p" at the top? We'll always use an explicit interpreter.
  2. In newpullpipe, since it's not designed to be importable, I would prefer at the top "if name != 'main': blow up" (where blow up is some exception), so nobody imports it by mistake.
  3. I'm a belt-and-suspenders kind of person, so in NewSendMsgTests.test_sendSubProcessFD, I would like the closing of the pipes to be more guaranteed. Before the pipe() method, why not do "self.addCleanup(lambda:close(pipeOut));self.addCleanup(lambda:close(pipeIn))" (except not lambdas, but real defs, and ignore os errors from the close). This way there is no chance for pipe() to be called without the cleanup functions already being there.

Please fix the issues in the first part (1-5) and resubmit.

comment:5 Changed 3 years ago by hawkowl

Owner: hawkowl deleted

Hi moshez, thanks for the review!

  1. I changed it to repr() the bytes, so it'll try to encode and otherwise do surrogate things, like elsewhere in Python. This is only a test, so it shouldn't impact people.
  2. Renamed. Worth noting that the new API works on both Py2 *and* 3, but just uses the stdlib on 3 and our module on 2.
  3. Double-close on all Python file-like objects is safe.
  4. This test isn't Py3 only, just tests the 2/3 API. I added a comment about the test.
  5. I added a compat function that makes a bytes environment, so that this works nicely in Python 3 (t.i.process gets mad if you give it text). This no longer nukes the environment, but I *am* in favour of nuking the PYTHONPATH.
  6. Don't need it, agreed.
  7. I don't think this is good -- it only has side effects if you call a function, and it's already in a test module.
  8. I think this is unneeded -- the pipes will be closed when theyre GC'd, and as it's a test, the process will shut down probably less than a minute later, so probably not worth it. :)

Builders spun and are a lovely shade of green, please review.

comment:6 Changed 3 years ago by hawkowl

Keywords: review added

comment:7 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to hawkowl

I think this is unneeded -- the pipes will be closed when theyre GC'd, and as it's a test, the process will shut down probably less than a minute later, so probably not worth it. :)

The pipe file descriptors will not be closed when they're garbage collected. The pipe file descriptors are integers. Nothing special happens when integers are garbage collected (if they're small integers they may even be immortal).

Even if they were closed when they were garbage collected, leaking resources out the end of a test method is bad form and relying on the garbage collector to free limited resources resources puts you at risk of resource exhaustion (consider trial --until-failure) and, when the garbage collection encounters an error, creates unrelated test failures (freaky action at a distance).

You should always explicitly clean up such resources.

comment:8 Changed 3 years ago by hawkowl

The Python 3 test runner complains when there's non-cleaned-up resources, and right now there's none left open (I removed the tearDown def and saw a bunch of warnings to make sure). Adding them to addCleanup means that the tests fail on Py2 (the poll reactor has a _pruneDescriptors or something which makes it not work).

So right now, when the tests fail, some FDs might not be cleaned up. Everything else is cleaned up correctly. Does this satisfy your concerns? If not, then I'm not sure how to ensure that they're closed because doing addCleanup causes Py2 failures.

comment:9 Changed 3 years ago by Jean-Paul Calderone

The Python 3 test runner complains when there's non-cleaned-up resources, and right now there's none left open

This is incorrect. It complains when resources with GC-triggered cleanup code are GCed. Since integers don't have any GC-triggered cleanup code, the Python 3 test runner doesn't complain about them.

Adding them to addCleanup means that the tests fail on Py2 (the poll reactor has a _pruneDescriptors or something which makes it not work).

I don't know why this would only be a problem on Python 2 and not on Python 3. Regardless of that, though, if it's really necessary, I'm sure you can add error handling to the cleanup code to prevent those errors.

comment:10 Changed 3 years ago by hawkowl

I figured out why it's a problem on Py2 and not 3:

The reactor does this:

            try:
                os.close(fd)
            except IOError:
                pass

On py3, if I add a cleanup for an already-closed FD, it works on Py3 and fails on Py2 because it raises OSError: [Errno 9] Bad file descriptor, and IOError/OSError are the same on Py3.3+:

Python 2.7.9 (default, Feb 10 2015, 03:28:08)
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.56)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> OSError == IOError
False
Python 3.4.2 (default, Oct 19 2014, 17:52:17)
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.51)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> IOError == OSError
True

So, I'm not sure what the *right* thing to do here is. If the reactor tries to close a bad FD (because it's been cleaned up already in this case), should it raise (as it does on Py2) or pass (as it does on Py3)?

comment:11 Changed 3 years ago by hawkowl

(the traceback on py2)

[ERROR]
Traceback (most recent call last):
  File "/Users/red/code/twgit/twisted/trial/_asynctest.py", line 209, in _cleanUp
    clean = util._Janitor(self, result).postCaseCleanup()
  File "/Users/red/code/twgit/twisted/trial/util.py", line 98, in postCaseCleanup
    calls = self._cleanPending()
  File "/Users/red/code/twgit/twisted/internet/utils.py", line 218, in warningSuppressingWrapper
    return runWithWarningsSuppressed(suppressedWarnings, f, *a, **kw)
  File "/Users/red/code/twgit/twisted/internet/utils.py", line 201, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/Users/red/code/twgit/twisted/internet/utils.py", line 197, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/Users/red/code/twgit/twisted/trial/util.py", line 139, in _cleanPending
    reactor.iterate(0)
  File "/Users/red/code/twgit/twisted/trial/_asynctest.py", line 242, in _
    return self._reactorMethods[name](*a, **kw)
  File "/Users/red/code/twgit/twisted/internet/base.py", line 632, in iterate
    self.doIteration(delay)
  File "/Users/red/code/twgit/twisted/internet/selectreactor.py", line 127, in doSelect
    self._preenDescriptors()
  File "/Users/red/code/twgit/twisted/internet/selectreactor.py", line 88, in _preenDescriptors
    self._disconnectSelectable(selectable, e, False)
  File "/Users/red/code/twgit/twisted/internet/posixbase.py", line 260, in _disconnectSelectable
    selectable.connectionLost(failure.Failure(why))
  File "/Users/red/code/twgit/twisted/internet/posixbase.py", line 157, in connectionLost
    os.close(fd)
exceptions.OSError: [Errno 9] Bad file descriptor

twisted.python.test.test_sendmsg.CModuleSendmsgTests.test_sendSubProcessFD

comment:12 Changed 3 years ago by hawkowl

After like two hours of investigating, I totally don't have a clue what is or isn't working here, so I'm just going to give up. Cleaning up the FDs in the test means that the reactor can't close them, so I'm unsure how to make sure they're always cleaned up.

comment:13 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Since I've not a clue about how I can do this, putting it up for review in hope that a reviewer will be able to give me an idea.

comment:14 Changed 3 years ago by hawkowl

Branch: branches/sendmsg-py3-7884branches/sendmsg-py3-7884-2

(In [44865]) Branching to sendmsg-py3-7884-2.

comment:15 Changed 3 years ago by hawkowl

Thanks to some help from Glyph, I figured out the exception. Because it was closed in the test, the cleanup was closing a FD (with the same number, because of aggressive reuse) made by the reactor, and so made everything fall apart.

I've fixed that, tests are green, please review.

comment:16 Changed 3 years ago by Moshe Zadka

Keywords: review removed
Owner: set to hawkowl

Hi HawkOwl,

Thanks for working on this.

I think this can go in. I only have one cosmetic comment: In bytesEnvironment, "return dict(os.environ)" -- I think os.environ.copy() is nicer, but that might be a style preference.

Feel free to either check-in as is, or change the issue above and check-in.

Moshe Z.

comment:17 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44905]) Merge sendmsg-py3-7884-2: Port twisted.python.sendmsg to Python 3

Author: hawkowl Reviewers: moshez, exarkun Fixes: #7884

comment:18 Changed 18 months ago by Craig Rodrigues

Resolution: fixed
Status: closedreopened

This module isn't fully ported to Python 3. If I try to build all the tests on Python 3, I get this C compilation error under MacOS X:

creating build/temp.macosx-10.11-x86_64-3.6/twisted/python
clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers -I/usr/local/include -I/usr/local/opt/openssl/include -I/usr/local/opt/sqlite/include -I/Users/crodrigues/twisted_env/include -I/usr/local/Cellar/python3/HEAD/Frameworks/Python.framework/Versions/3.6/include/python3.6m -c twisted/python/_sendmsg.c -o build/temp.macosx-10.11-x86_64-3.6/twisted/python/_sendmsg.o
twisted/python/_sendmsg.c:126:14: warning: implicit declaration of function 'Py_InitModule3' is invalid in C99 [-Wimplicit-function-declaration]
    module = Py_InitModule3("_sendmsg", sendmsg_methods, sendmsg_doc);
             ^
twisted/python/_sendmsg.c:126:12: warning: incompatible integer to pointer conversion assigning to 'PyObject *' (aka 'struct _object *') from 'int'
      [-Wint-conversion]
    module = Py_InitModule3("_sendmsg", sendmsg_methods, sendmsg_doc);
           ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
twisted/python/_sendmsg.c:129:9: error: non-void function 'init_sendmsg' should return a value [-Wreturn-type]
        return;
        ^
twisted/python/_sendmsg.c:138:9: error: non-void function 'init_sendmsg' should return a value [-Wreturn-type]
        return;
        ^
twisted/python/_sendmsg.c:145:9: error: non-void function 'init_sendmsg' should return a value [-Wreturn-type]
        return;
        ^
twisted/python/_sendmsg.c:159:9: error: non-void function 'init_sendmsg' should return a value [-Wreturn-type]
        return;
        ^
twisted/python/_sendmsg.c:165:9: error: non-void function 'init_sendmsg' should return a value [-Wreturn-type]
        return;
        ^
twisted/python/_sendmsg.c:170:9: error: non-void function 'init_sendmsg' should return a value [-Wreturn-type]
        return;
        ^

comment:19 Changed 18 months ago by hawkowl

Resolution: fixed
Status: reopenedclosed

As mentioned on IRC; that module is no longer needed on Py3 as this patch makes us use the stdlib version.

Note: See TracTickets for help on using tickets.