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.
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
[9410] Don't throw KeyError exception on write to _disconnected http request #1106
Changes from 9 commits
169fd1d
4148d41
fe30de6
8a217b2
31f18c5
db18f5d
8441750
1e7c194
9f052df
00a5979
bade1d9
41c1592
582465d
4d7733c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.