Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#9653] Make HTTP/2 server connections not timeout after a minute #1158

Merged
merged 6 commits into from
Aug 18, 2019
Merged

[#9653] Make HTTP/2 server connections not timeout after a minute #1158

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0114d80
Make the transport properly carry over the timeout
hawkowl Jun 15, 2019
1f6950b
fix the test
hawkowl Jun 15, 2019
289d596
fixes
hawkowl Jun 16, 2019
2487b32
Merge branch 'trunk' into 9653-http2-timeouts
markrwilliams Aug 9, 2019
c0def76
No new public API; use Clock and hamcrest in tests.
markrwilliams Aug 9, 2019
4adb0b1
Merge branch 'trunk' into 9653-http2-timeouts
twm Aug 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/twisted/newsfragments/9653.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Incoming HTTP/2 connections will now not time out if they persist for longer than one minute.
16 changes: 16 additions & 0 deletions src/twisted/protocols/policies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,22 @@ def resetTimeout(self):
if self.__timeoutCall is not None and self.timeOut is not None:
self.__timeoutCall.reset(self.timeOut)


def cancelTimeout(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the one hand this is a new public API that isn't necessary; passing None to setTimeout will cancel the timeout.
On the other hand, setTimeout's API is bad enough that a core committer with deep Twisted experience didn't see it, and I didn't either until I went hunting. Maybe if we'd had this method in the first place we wouldn't have had this bug!
So I think I'm +0.75 for adding this method.
Should we deprecate the cancellation feature of setTimeout?

"""
Cancel the timeout.

If the timeout was already cancelled, this does nothing.
"""
self.timeOut = None
if self.__timeoutCall:
try:
self.__timeoutCall.cancel()
except (error.AlreadyCalled, error.AlreadyCancelled):
pass
self.__timeoutCall = None


def setTimeout(self, period):
"""
Change the timeout period
Expand Down
3 changes: 3 additions & 0 deletions src/twisted/web/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2947,6 +2947,9 @@ def dataReceived(self, data):
# itself if possible.
self._channel._networkProducer.unregisterProducer()

# Cancel the old channel's timeout.
self._channel.cancelTimeout()

transport = self._channel.transport
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about removing the transport from the original channel as well, for a bit of defensive programming?

self._channel = H2Connection()
self._channel.requestFactory = self._requestFactory
Expand Down
60 changes: 59 additions & 1 deletion src/twisted/web/test/test_http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@

from __future__ import absolute_import, division

import random, cgi, base64, calendar
import attr
import base64
import calendar
import cgi
import random

try:
from urlparse import urlparse, urlunsplit, clear_cache
Expand Down Expand Up @@ -764,6 +768,60 @@ def _negotiatedProtocolForTransportInstance(self, t):
return a._negotiatedProtocol


def test_h2CancelsH11Timeout(self):
"""
When the transport is switched to H2, the HTTPChannel timeouts are
cancelled.
"""
@attr.s
class FakeDelayedCall(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not construct a real DelayedCall?

Also, it'd be nice if this implemented IDelayedCall and there were a verifyObject call somewhere.

period = attr.ib()
callback = attr.ib()
cancelled = attr.ib(default=False)

def cancel(self):
self.cancelled = True

def reset(self, period):
self.period = period

delayedCalls = []

def fakeCallLater(period, callback):
c = FakeDelayedCall(period, callback)
delayedCalls.append(c)
return c

a = http._genericHTTPChannelProtocolFactory(b'')
a.requestFactory = DummyHTTPHandlerProxy

# Set the original timeout to be 100s
a.timeOut = 100
a.callLater = fakeCallLater

b = StringTransport()
b.negotiatedProtocol = b'h2'
a.makeConnection(b)

# We've made the connection, but we actually check if we've negotiated
# H2 when data arrives. Right now, the HTTPChannel will have set up a
# single delayed call.
self.assertEqual(len(delayedCalls), 1)
self.assertFalse(delayedCalls[0].cancelled)

# We give it the HTTP data, and it switches out for H2.
a.dataReceived(b'')
self.assertEqual(a._negotiatedProtocol, b'h2')

# The first delayed call is cancelled, and H2 creates a new one for its
# own timeouts.
self.assertEqual(len(delayedCalls), 2)
self.assertTrue(delayedCalls[0].cancelled)
self.assertFalse(delayedCalls[1].cancelled)
if not http.H2_ENABLED:
test_h2CancelsH11Timeout.skip = "HTTP/2 support not present"


def test_protocolUnspecified(self):
"""
If the transport has no support for protocol negotiation (no
Expand Down