Wrap ConnectionLost in a Failure for HTTP/2

[clokep]

Contributor Checklist:

• The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9817
• The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
• I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
• I have updated the automated tests.
• I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

[richvdh]

@clokep it looks like there are other places this is happening - would it be worth cleaning up the others while we are here?

[clokep]

@richvdh I'll take another look and see if I can find more.

[glyph]

Thanks so much for your contribution! This does look like an obviously correct fix, but we should really have some automated test changes to go along with it to make sure we keep this behavior.

The good news is that I believe this code is already tested, it's just not looking at the values of its arguments - so if you could just add an assertion or two, that would be great!

[clokep]

@glyph Thanks for the feedback! I took a quick look at the test framework and starting dissecting one of the tests:

https://github.com/clokep/twisted/blob/ad67f70a79830f68ef35b5032959a95291daf29e/src/twisted/web/test/test_http2.py#L1572-L1611

Weirdly this seems to be checking for the desired type already. (This would be to match the change here: https://github.com/twisted/twisted/pull/1265/files#diff-4872b70e1efa2de2dd576dff031f2ef0L352 ).

I've traced the value through to the point where it hits _IDeprecatedHTTPChannelToRequestInterfaceProxy and then I kind of get lost, but it somehow pops out in the list of Deferreds in NotifyingRequestFactory.results. Any hints on what I'm missing here? I'm wondering if something in the testing framework is turning it into a Failure for us here!

[glyph]

@clokep You're quite welcome!

I might not have time soon to dive into this question, so I'm going to page dr. @Lukasa to the thread to see if he can quickly recall some nuance of the http2 test setup that would make diagnosing this easy.

[clokep]

I spent a bit more time debugging this and what is happening on trunk is the following (at least for the test case I was looking at, twisted.web.test.test_http2.HTTP2ServerTests.test_failOnStopProducing):

• The test case directly calls H2Connection.stopProducing().
• H2Connection.connectionLost() is called with a ConnectionLost() instance.
• Each H2Stream is iterated and has connectionLost() called on it (with whatever was passed into it, so still ConnectionLost()).
• The stream's request has connectionLost() called (again proxy the reason). The request will be whatever the requestFactory creates, but is likely a sub-class of twisted.web.http.Request (that's at least the case we care about here).
• twisted.web.http.Request.connectionLost iterates through the Deferred instances which are waiting to be notified and calls errback() on each with the same reason from above (still a ConnectionLost()).
• The Deferred.errback code path automatically converts non-Failure instances into a Failure by wrapping whatever is passed in.

So I think this normally works "fine", but isn't fully correct. Note that the reason Synapse hit this (matrix-org/synapse#7441) is because a custom Request sub-class is used that expects connectionLost to be called with a Failure.

Unfortunately I'm not sure this gets me closer to being able to test this, but explains why the tests already see this as working correctly.

[clokep]

I should likely summarize my issue a bit better: I'm unsure how to add an assertion that connectionLost gets called properly since the tests seem to all go through a Deferred chain eventually which automatically converts things to a Failure instance. Usually I would consider doing this by using mocks or something, but not sure if that's appropriate here? Or maybe a custom request sub-class that asserts the input to connectionLost is a Failure?

[glyph]

I'm unsure how to add an assertion that connectionLost gets called properly since the tests seem to all go through a Deferred chain eventually which automatically converts things to a Failure instance.

So, the way to ask this is "for whom is it a problem", i.e. what is it that might application code do where it will see the difference?

As it is… I'm not actually sure that public application code can see this. A subclass of H2Connection might see the distinction, but… you can't actually subclass H2Connection without importing a private implementation detail, not covered in the documentation or exported in __all__?

For type-correctness we should still be doing this, because when we type-hint connectionLost it'll say it takes a Failure but it's not being passed that here. So there are two ways around this:

1. add some type hints and reduce the number of resulting mypy failures, then fix those failures, rather than adding tests
2. add some tests that subclass H2Connection and override connectionLost to capture its argument and then assert about it, even though it's not technically publicly exposed.
3. use a Failure with a traceback and assert something about the traceback, since the auto-creation of Failures from exceptions will make them tracebackless. Not sure if this is really feasible or even desirable in this context though.

Given that this is effectively an internal implementation detail though, not something that the public interface of Twisted actulaly exposes, I would say that the news fragment ought to be a .misc.

[clokep]

add some type hints and reduce the number of resulting mypy failures, then fix those failures, rather than adding tests

This seems like the most promising approach to me -- unfortunately mypy is currently complaining about the signature, which I think masks any other issues with this method.

src/twisted/web/_http2.py:270:5: error: Signature of "connectionLost" incompatible with supertype "Protocol" [override]

I'll look at it more though, that was just from a quick test!

[rodrigc]

This change looks correct.

In your tree, before pushing to GitHub, can you run:

tox -e lint

And fix a few style errors (lines too long) which are showing up as errors during CI?

[clokep]

And fix a few style errors (lines too long) which are showing up as errors during CI?

I think I had a chat over IRC about this originally...the changes kind of snowball once I do that, but it was easy enough!

I've pushed a commit which fixes lint + merged in trunk again.

[rodrigc]

Looks good. Lint now passes

[rodrigc]

@clokep I think if more of the functions in Twisted had function annotations, and we ran mypy more regularly, this would help more quickly find the kinds of bugs which you fixed.
Take a look at: #1293

[clokep]

@rodrigc I definitely agree! I'm glad to see that Twisted is starting to adopt type annotations. I've find them to be very useful in a large-ish project. (Especially ones where you care about bytes vs. str.) Thanks for working on this (and for the review!)

[clokep]

Thanks for all the help! 🎉