Ticket #1955 defect new

Opened 8 years ago

Last modified 3 years ago

a forgotten resetTimeout in twisted.protocols.postfix

Reported by: quakelee Owned by:
Priority: low Milestone:
Component: core Keywords:
Cc: jesstess, thijs Branch: branches/postfix-timeout-1955
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description (last modified by thijs) (diff)

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

    old new  
    4848    def sendCode(self, code, message=''): 
    4949        "Send an SMTP-like code with a message." 
    5050        self.sendLine('%3.3d %s' % (code, message or '')) 
     51        self.resetTimeout() 
    5152 
    5253    def lineReceived(self, line): 
    5354        self.resetTimeout() 

Attachments

patch-postfix.py Download (357 bytes) - added by quakelee 8 years ago.
patch of postfix.py

Change History

1

  Changed 8 years ago by 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.

2

  Changed 8 years ago by 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.

Changed 8 years ago by quakelee

patch of postfix.py

3

  Changed 8 years ago by 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.

4

  Changed 8 years ago by 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.

5

  Changed 5 years ago by glyph

  • owner changed from glyph to quakelee

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!

6

  Changed 4 years ago by jesstess

  • branch set to branches/postfix-timeout-1955
  • branch_author set to jesstess

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

7

  Changed 4 years ago by 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

8

  Changed 4 years ago by jesstess

  • keywords review added
  • owner quakelee deleted
  • cc jesstess added

9

  Changed 4 years ago by exarkun

  • description modified (diff)

fixing description markup

10

follow-up: ↓ 12   Changed 4 years ago by exarkun

  • keywords review removed
  • owner set to jesstess

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.

11

  Changed 4 years ago by thijs

  • cc thijs added
  • description modified (diff)

12

in reply to: ↑ 10   Changed 3 years ago by thijs

  • owner changed from jesstess to exarkun

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.

13

  Changed 3 years ago by exarkun

  • owner exarkun deleted

I'm not going to work on this.

14

  Changed 3 years ago by <automation>

Note: See TracTickets for help on using tickets.