Opened 15 years ago

Closed 12 years ago

Last modified 12 years ago

#2004 enhancement closed wontfix (wontfix)

t.i.tcp._SocketCloser should not do the shutdown trick

Reported by: ghazel Owned by: ghazel
Priority: normal Milestone:
Component: core Keywords:
Cc: jknight, spiv, ghazel, titty Branch:
Author:

Description

The shutdown trick in twisted.internet.tcp._SocketCloser is not needed, and breaks apps that rely on TCP/IP inheritance. Twisted already sets FD_CLOEXEC, that should be good enough. On Windows you can say bInheritHandles=False in CreateProcess.

Change History (18)

comment:1 Changed 15 years ago by Jean-Paul Calderone

Resolution: invalid
Status: newclosed

Apps that rely on this should completely remove the appropriate Twisted objects from the reactor so that Twisted does not try to clean them up.

_SocketCloser only gets invoked when Twisted decides it is time for a socket to die. When Twisted decides that, the socket should actually die.

comment:2 Changed 15 years ago by ghazel

Changing this code to only close the socket allows the rest of the cleanup process to work fine. Apps that rely on sockets dying should not dupe them, or close all instances. What you're doing here is breaking the flexibility of handle inheritence because it's hard to manage.

comment:3 Changed 15 years ago by ghazel

Resolution: invalid
Status: closedreopened
Type: defectenhancement

At the very least, there should be "new" functionality which makes transports that wrap sockets act like normal sockets. Perhaps even something like:

transport.close()

comment:4 Changed 15 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to ghazel
Status: reopenednew

I still don't understand the use case here.

If you don't want the reactor to mangle a socket, why give the socket to the reactor at all? If the reactor already has a socket and you don't want it mangled, why not remove the socket from the reactor?

comment:5 Changed 15 years ago by jknight

Cc: jknight added

I agree with ghazel, the shutdown call is redundant. If it can be removed without breaking things, it should be removed.

I've thought it should be removed for a while, since I was looking at #78. You can't call shutdown if you want to abort the connection, so it'd be good to get rid of it in the normal case too for consistency.

comment:6 Changed 15 years ago by spiv

Cc: spiv added

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

Cc: ghazel added

I'm not sure that consistency is worth anything. I don't necessarily object to removing the shutdown call if that is correct (I don't remember that part of the code extremely well, but I remember why we added it in the first place, and it was pretty important at the time), and if you've looked and think it is correct I'll take your word for it for now.

But I'm still curious about the use case for this. What are the apps which are broken by this feature doing?

comment:8 in reply to:  7 Changed 15 years ago by ghazel

If you added it before FD_CLOEXEC then it would seem necessary. It is not now, however.

The application is one that does fd passing.

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

Even if the shutdown() call is removed, you still won't be able to pass file descriptors between processes reliably unless you remove the socket from the reactor entirely. And if you remove the socket from the reactor entirely, shutdown() won't be called on it.

comment:10 in reply to:  9 Changed 15 years ago by ghazel

Removing the shutdown call and using transport.loseConnection() removes the socket from the reactor to my satisfaction. How is this incomplete?

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

That is not the purpose of transport.loseConnection(). It shouldn't be co-opted for this unrelated feature.

comment:12 in reply to:  11 Changed 15 years ago by ghazel

So what should the new function be called?

transport.close() transport.disconnect() transport.loseTrackOfConnection() transport.disassociate()

The BSD socket api calls it close()

comment:13 Changed 15 years ago by Jean-Paul Calderone

The functions which do this exist already, reactor.removeReader and reactor.removeWriter. They are part of no public interface, but they are more along the lines of how this feature should be offered than a method on transport would be.

See #1796

comment:14 Changed 15 years ago by jknight

Regardless of whether or not ghazel's desired use is proper or not, I _still_ think we should not be calling shutdown. I am definitely in favor of merging a patch that makes that so, assuming that tests still pass.

comment:15 Changed 15 years ago by titty

Cc: titty added

I've recently stumbled upon this one in a real world application. I'm opening a listening socket and then fork several child processes, which accept new connections. The parent does not accept new connections, but kills those children eventually (after having loaded new data), and then starts some new ones. When one of the children does the "shutdown trick", the listening socket becomes unusable in all processes.

Why was the shutdown call added in the first place?

comment:16 Changed 12 years ago by Glyph

Resolution: wontfix
Status: newclosed

As specified, this ticket is kind of bogus.

Pretty much everybody on this ticket seems to be talking about one form of file descriptor inheritance or another. If you want to inherit file descriptors or pass them between processes, let's have a public API for doing that. shutdown() is totally the right thing to do in the case of connection-oriented sockets, which can't really be shared; I can understand the use-case for a Port shared between multiple processes, but that's different, and should be addressed with a specific API, not by mucking around in the internals to change the semantics of existing functionality.

I personally kind of want the shutdown call to stay, because it's easy to leak a file descriptor in many situations (socket.fromfd implicitly calling dup, in particular) and it's nice that Twisted cleans up after that and really shuts down the connection.

So, please go file tickets about APIs for inheriting file descriptors, we definitely need better ways to do that; perhaps one of them will be even resolved by (among other things) getting rid of the call to shutdown, but in itself, I don't think this is a worthwhile goal.

comment:17 Changed 12 years ago by Jean-Paul Calderone

I personally kind of want the shutdown call to stay, because it's easy to leak a file descriptor in many situations (socket.fromfd implicitly calling dup, in particular) and it's nice that Twisted cleans up after that and really shuts down the connection.

Calling shutdown doesn't protect you from leaking a file descriptor if someone calls dup. You still have to close them both (although, what the profanity, why does socket.fromfd do such an obviously incorrect thing). It only prevents the connection from staying up, which maybe is useful, I guess, I dunno. It's certainly a behavior that's currently provided by the API, at least.

comment:18 in reply to:  17 Changed 12 years ago by Glyph

Replying to exarkun:

I personally kind of want the shutdown call to stay, because it's easy to leak a file descriptor in many situations (socket.fromfd implicitly calling dup, in particular) and it's nice that Twisted cleans up after that and really shuts down the connection.

Calling shutdown doesn't protect you from leaking a file descriptor if someone calls dup.

Sorry, I didn't mean to imply that it did. It just protects you from leaking an open connection; which, if you're talking about a Port object, and you're only leaking one FD, but blocking all future daemons from starting up again because of one errant subprocess, is still broken but far less annoying.

(what the profanity, why does socket.fromfd do such an obviously incorrect thing).

I know, right!??!

It's certainly a behavior that's currently provided by the API, at least.

On that, I hope we can all agree.

Note: See TracTickets for help on using tickets.