a forgotten resetTimeout in twisted.protocols.postfix

[quakelee]

I think after sent code to client, daemon should reset timeout. If data source is a big and slow object, getting and sending result to client may consume a lot of time, if you didn't reset the timer, the connection maybe lost before client reply to daemon.

postfix.py

@@ -48 +48 @@
     def sendCode(self, code, message=''):
         "Send an SMTP-like code with a message."
         self.sendLine('%3.3d %s' % (code, message or ''))
+        self.resetTimeout()
 
     def lineReceived(self, line):
         self.resetTimeout()

[quakelee]

I think after sent code to client, daemon should reset timeout. If data source is a big and slow object, getting and sending result to client may consume a lot of time, if you didn't reset the timer, the connection maybe lost before client reply to daemon.

[exarkun]

The server should actually be disabling the timeout before it begins any long response and re-enabling it after it has completely finished sending it. Resetting the timeout afterwards is insufficient, since the timeout may occur before the send even finishes.

[quakelee]

patch of postfix.py

[quakelee]

In our enviroment, we use the ldap server as a data source. Unfortunately, the ldap server not very stable. Sometimes it can query very fast, but sometimes maybe consume a longtime. So we add a resetTimeout to prevent unexpected lost connection.

BTW: we discuss the problem about how to deal connection when daemon meet a error have to reply 400 to client with the inventor of postfix tcpmap. After discussion we think we should close the connection directly instead code 400. If do that, postfix will retry it.

[quakelee]

In the other hand, If some people want to shorten the timeout time, I think reset timeout after sending code is more important than it is 600s or longer.

[glyph]

Thanks for your contribution. Sorry we've been so long in getting to it. Please see the TwistedDevelopment page on the wiki for guidelines on how to get a more prompt response.

Please note that this patch will need test coverage in order to be accepted. I hope you will add some and submit it for review!

[jesstess]

(In [28108]) Branching to 'postfix-timeout-1955'

[jesstess]

(In [28109]) Disable the timeout in sendCode before calling sendLine and re-enable it afterwards to avoid timing out while the server is sending a status message.

refs #1955

[exarkun]

fixing description markup

[exarkun]

Disabling the timeout around a synchronous call to sendLine won't do anything useful. sendLine is just a trivial wrapper around ITCPTransport.write, which always succeeds immediately.

The timeout logic here is basically a timeout on how long the server will wait to receive data. It should only be active when the server is actually expecting to receive something from the client. That basically equates to being active in two cases:

1. between when the connection is initially set up and the first line is received
2. between the sendCode call answering one request and the first line being received (representing the next request)

To me, this suggests that lineReceived should start with setTimeout(None) and sendCode should end with setTimeout(self.timeout). I think the unit test should probably:

1. set up a fake clock (monkey patch the callLater method of PostfixTCPMapServer)
2. implement a new do_ method that returns a Deferred that can be fired by the test at an arbitrary time
3. deliver some bytes to the protocol so it will invoke that new do_ method
4. advance the clock more than the timeout and assert that the connection isn't dropped
5. fire the Deferred the do_ method returned
6. assert the timeout is renabled (eg advance the clock by more than it and verify the connection is dropped)

Thanks.

[thijs]

Replying to exarkun:

To me, this suggests that lineReceived should start with setTimeout(None) and sendCode should end with setTimeout(self.timeout). I think the unit test should probably: 1. set up a fake clock (monkey patch the callLater method of PostfixTCPMapServer) 1. implement a new do_ method that returns a Deferred that can be fired by the test at an arbitrary time 1. deliver some bytes to the protocol so it will invoke that new do_ method 1. advance the clock more than the timeout and assert that the connection isn't dropped 1. fire the Deferred the do_ method returned 1. assert the timeout is renabled (eg advance the clock by more than it and verify the connection is dropped) Thanks.

[exarkun]

I'm not going to work on this.