Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#3226 enhancement closed fixed (fixed)

twisted.python.util.initgroups (and thus spawnProcess) potentially does tons of unnecessary IO

Reported by: radix Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, Glyph, Jean-Paul Calderone, Thijs Triemstra Branch: branches/initgroups-3226
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

See this post by Phil Mayers, five years ago on TPML:

http://twistedmatrix.com/pipermail/twisted-python/2003-April/003674.html

Apparently with certain authentication setups, several tens of megabytes of I/O can happen in response to a "getgrall" call. We should probably try using the low-level C "initgroups" function before falling back to an inefficient version that uses getgrall.

Change History (17)

comment:1 Changed 10 years ago by therve

Cc: therve added

So, we hit this bug in Landscape. I see several options here:

comment:2 Changed 10 years ago by Glyph

Cc: Glyph added

I would personally appreciate it if you would integrate the CalendarServer version, especially as that will be easier to deploy on most modern Pythons than an additional dependency :).

comment:3 Changed 10 years ago by therve

The problem with the ctypes version is that it doesn't raise any exception because of the lack of errno handling. I was thinking of copying the C implementation inside Twisted, if the license is compatible.

comment:4 Changed 10 years ago by Glyph

The ctypes implementation could be fixed, though, right?

comment:5 Changed 10 years ago by therve

I don't know how to do that, though.

comment:6 Changed 10 years ago by Glyph

Owner: changed from Glyph to therve

I thought I did, but then I found http://bugs.python.org/issue1798 and now I think that maybe it's impossible to do error handling in ctypes on python<2.6. So, C it is, I guess. The backport idea seems okay. Reassigning to you if you want to take care of it?

comment:7 Changed 10 years ago by therve

Author: therve
Branch: branches/initgroups-3226

(In [27508]) Branching to 'initgroups-3226'

comment:8 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

I did something. Testing this sucks though.

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

Keywords: review removed
Owner: set to therve

I pushed the initgroups code upstream: <http://bugs.python.org/issue7333>. Hopefully it will land for 2.7 and we'll only have to maintain this C code for 7 - 10 years.

So other stuff:

  1. The test_initgroupsInC docstring has a mistake in it.
  2. All the InitGroupsTests skip if _c_initgroups isn't available, but given what test_initgroupsForceC and test_initgroupsForcePython do, I would expect these to run all the time, since they don't actually rely on the _c_initgroups implementation. test_initgroupsInC seems to, but I'm not sure it needs to. Actually, I'm not sure how what it tests is different from what test_initgroupsForceC tests.
  3. Can you update the initgroups docstring to mention that it might call the platform-supplied initgroups now?

Otherwise, seems pretty reasonable. Maybe also put a link to the CPython ticket into the C source as a warning to people who want to modify the code (though I really hope there won't be any such people) about divergence.

comment:10 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

Thanks for the review! I fixed the docstring typo. The second skip was indeed wrong, I intended to only skip test_initgroupsForceC. The difference with the other test is that it actually calls the C extension, preventing stupid mistakes like causing a segfault. I'd rather check that the underlying call is done, but it would probably involve black magic.

comment:11 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Cool. One last thing, please change setgroups_calls in test_initgroupsForceC. Then go ahead and merge.

comment:12 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(In [27775]) Merge initgroups-3226: add C initgroups implementation.

twisted.protocols.amp now provides a ListOf argument type which can be composed with some other argument types to create a zero or more element sequence of that type.

Author: therve Reviewer: exarkun Fixes: #3226

comment:13 Changed 10 years ago by Thijs Triemstra

Cc: Jean-Paul Calderone Thijs Triemstra added

So that C initgroups implementation has a Zope License. I'm confused on how this could land in the trunk, why was it not updated to use Twisted's license?

comment:14 Changed 10 years ago by Thijs Triemstra

Ok I found this statement but that only talks about the PSF license, not the MIT. I know they're similar licenses but this still feels wrong..

comment:15 Changed 10 years ago by therve

Well it can't have Twisted's license because it's from Zope? AFAICT this is a compatible license though. We can't modify the license like that.

comment:16 in reply to:  15 Changed 10 years ago by Thijs Triemstra

Replying to therve:

Well it can't have Twisted's license because it's from Zope? AFAICT this is a compatible license though. We can't modify the license like that.

I see what you mean but perhaps below the license header it should have a reference to that mailing list post or something. Guess it's too late now. Maybe something that should be added to, or more explicitly defined in, the coding-standard.

comment:17 Changed 9 years ago by <automation>

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