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

Jump to bottom

[9410] Don't throw KeyError exception on write to _disconnected http request #1106

Merged
merged 14 commits into from
May 17, 2019
Merged

[9410] Don't throw KeyError exception on write to _disconnected http request #1106

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
169fd1d
Don't write data to disconnected http clients
Feb 27, 2019
4148d41
Add newsfile fragment for trac 9410 fix
Feb 27, 2019
fe30de6
Don't expect AttributeError on write after disconnect
Feb 27, 2019
8a217b2
fix trailing whitespace
Feb 27, 2019
31f18c5
Don't pass through loseConnection to null channel
Mar 1, 2019
db18f5d
Merge branch 'trunk' into trunk
Mar 4, 2019
8441750
Merge branch 'trunk' into trunk
glyph Apr 7, 2019
1e7c194
Merge branch 'trunk' into trunk
Apr 9, 2019
9f052df
Merge branch 'trunk' into trunk
glyph Apr 18, 2019
00a5979
Use more consistent language in news and test
jswitzer Apr 29, 2019
bade1d9
Merge branch 'trunk' into trunk
May 2, 2019
41c1592
Merge branch 'trunk' into trunk
twm May 8, 2019
582465d
Merge branch 'trunk' into trunk
May 13, 2019
4d7733c
Update newsfragment and fix None comparison
jswitzer May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/twisted/newsfragments/9410.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
write() to disconnected twisted.web.http.Request will no longer raise AttributeError.
jswitzer marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 9 additions & 1 deletion src/twisted/web/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,6 +1103,13 @@ def write(self, data):
if self.finished:
raise RuntimeError('Request.write called on a request after '
'Request.finish was called.')

if self._disconnected:
# Don't attempt to write any data to a disconnected client.
# The RuntimeError exception will be thrown as usual when
# request.finish is called
return

if not self.startedWriting:
self.startedWriting = 1
version = self.clientproto
Expand Down Expand Up @@ -1586,7 +1593,8 @@ def loseConnection(self):
"""
Pass the loseConnection through to the underlying channel.
"""
self.channel.loseConnection()
if self.channel:
jswitzer marked this conversation as resolved.
Show resolved Hide resolved
self.channel.loseConnection()


def __eq__(self, other):
Expand Down
13 changes: 13 additions & 0 deletions src/twisted/web/test/test_http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2932,6 +2932,19 @@ def test_finishAfterConnectionLost(self):
self.assertRaises(RuntimeError, req.finish)


def test_writeAfterConnectionLost(self):
"""
Calling L{Request.write} after L{Request.connectionLost} has been
called does not raise an exception. L{RuntimeError} will be raised
when finish is called on the request.
"""
channel = DummyChannel()
req = http.Request(channel, False)
req.connectionLost(Failure(ConnectionLost("The end.")))
req.write(b'foobar')
self.assertRaises(RuntimeError, req.finish)


def test_reprUninitialized(self):
"""
L{Request.__repr__} returns the class name, object address, and
Expand Down
5 changes: 3 additions & 2 deletions src/twisted/web/test/test_http2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,8 +946,9 @@ def test_terminatedRequest(self):
self.assertTrue(request._disconnected)
self.assertTrue(request.channel is None)

# An attempt to write should at this point raise an exception.
self.assertRaises(AttributeError, request.write, b"third chunk")
# This should not raise an exception, the function will just return
# immediately, and do nothing
request.write(b"third chunk")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. So this is actually a tested behavior of a public interface being changed... 🤔

Although, jeez, AttributeError? That's a pretty sad behavior. I expect whoever wrote this test just encoded whatever accidental implementation behavior they happened to get without thinking about it very hard.

Yet, regardless of that, it's still a tested behavior of a public interface. Strictly speaking, this is a backwards incompatible change (what if someone wrote an application that depends on AttributeError here to notice a closed connection?).

Ignoring backward compatibility concerns, I'd probably go along with this new behavior as sensible, but I don't think we get to ignore those concerns. Unless... considering people are complaining about this behavior after upgrading, it could be the exception behavior is relatively new and was introduced accidentally as part of some other work?

The "safe" thing for me to say is that this fix needs to be inside the wsgi implementation instead of inside the IRequest implementation. If we knew exactly when the exception behavior was introduced, maybe we could make a different decision.

And, with a little help from git bisect, I see that ticket:8191 is where the change was introduced and it was first released in 16.3.0. That's annoyingly long ago. Does it mean the new behavior has to be considered the intended behavior and compatibility needs to be maintained? Or do we still just call this a regression fix, even though the regression has been in the wild for 3 years & 10 releases...

Copy link
Contributor Author

@jswitzer jswitzer Apr 25, 2019 •
edited

Choose a reason for hiding this comment

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

Yes it definitely is an interface change (or more-precisely, change back?) and I suppose there could be projects out there that modified the interface they expected and then will have to modify it again.

I think some kind of behavior change is called for though, whether or not we throw a different kind of (more catcheable than AttributeError) Exception or go back to the old behavior (defer exception until finish()). This PR would be for the latter option.

That being said, the project I'm working on could go either way - we'd be happy to just catch and ignore a specific-enough exception. I'd like to see some change made at the IRequest implementation level (not just wsgi level) - the project I'm working on is calling request.write() directly and that's what started to fail for disconnected clients when we upgraded from twisted 15 -> 18.


# Check that everything is fine.
# We expect that only the Settings and Headers frames will have been
Expand Down