-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
[9410] Don't throw KeyError exception on write to _disconnected http request #1106
Merged
Merged
Changes from all 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
4148d41
Add newsfile fragment for trac 9410 fix
fe30de6
Don't expect AttributeError on write after disconnect
8a217b2
fix trailing whitespace
31f18c5
Don't pass through loseConnection to null channel
db18f5d
Merge branch 'trunk' into trunk
8441750
Merge branch 'trunk' into trunk
glyph 1e7c194
Merge branch 'trunk' into trunk
9f052df
Merge branch 'trunk' into trunk
glyph 00a5979
Use more consistent language in news and test
jswitzer bade1d9
Merge branch 'trunk' into trunk
41c1592
Merge branch 'trunk' into trunk
twm 582465d
Merge branch 'trunk' into trunk
4d7733c
Update newsfragment and fix None comparison
jswitzer File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
twisted.web.http.Request.write after the channel is disconnected will no longer raise AttributeError. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.