avoid calling req.finish() on closed HTTP connections

[warner]

While reviewing #393 MDMF, I ran into some test failures which were related to an HTTP "GET" connection being closed by the client before the tahoe server had a chance to close it itself. I think it only occurs during the new tests added as part of the #393 patch, but it's not MDMF-specific.

Here's a patch to fix it. Note that Twisted-9.0 was the first version which started raising an error when you call request.finish() on a request that's already been closed, and it was also the first version to offer request.notifyFinish() to tell you when it's been closed. So this patch has to switch on hasattr(req, "notifyFinish") until the time we raise our dependency on Twisted to 9.0 or later.

[warner]

patch to avoid test failures with the #393 tests, but appropriate for trunk

[zooko]

Could this have been related to #1278 (gateway won't serve any page; variety of interesting error messages in twistd.log)?

[davidsarah]

Looks good to me. (I only eyeballed it, I didn't run it.)

[davidsarah]

Replying to zooko:

Could this have been related to #1278 (gateway won't serve any page; variety of interesting error messages in twistd.log)?

That does look likely given the log message on #1278:

exceptions.RuntimeError: Request.finish called on a request after its
connection was lost; use Request.notifyFinish to keep track of this.

[zooko]

+1 on attachment:1366.dpatch​ . I would appreciate it if you could investigate #1278. Hopefully this fixes it! I seem to recall that it would happen when there were failures e.g. due to insufficient servers, and it looks like that might have gotten the Tahoe-LAFS web code into a bad state when Twisted >= 9.0 raised an exception from the Tahoe-LAFS web code's attempt to finish an already-finished connection, and that then the Tahoe-LAFS web code might have been unable to serve other requests after that.

Again, please remove the reviewed tag once you've pushed this patch to trunk (or suggest a different way to track that information).

[zooko]

Replying to davidsarah:

Replying to zooko:

Could this have been related to #1278 (gateway won't serve any page; variety of interesting error messages in twistd.log)?

That does look likely given the log message on #1278:

exceptions.RuntimeError: Request.finish called on a request after its
connection was lost; use Request.notifyFinish to keep track of this.

If my theory from comment:6 looks sufficiently likely to you then perhaps we should close #1278 as "MIA--presumed dead". It wasn't very reproducible by me -- perhaps there is an element of race condition between Twisted finishing the connection and Tahoe-LAFS finishing it ?

["Brian Warner <warner@…]

In 44466fbb1bad4ba0:

web/filenode.py: avoid calling req.finish() on closed HTTP connections. Closes #1366

[davidsarah]

The main bug in this ticket is fixed for 1.9. However since the Twisted dependency has been raised to >= 10.1, the hasattr switch is no longer needed.

[davidsarah]

web/filenode.py: since we depend on Twisted >= 10.1, there is no need to check for the existence of 'req.notifyFinish', which was added in Twisted 9.0. closes #1366

[Brian Warner <warner@…>]

In 587e31a8cf486031:

(The changeset message doesn't reference this ticket)

[Brian Warner <warner@…>]

In [5507/ticket999-S3-backend]:

web/filenode.py: rely on Request.notifyFinish. Closes #1366.

This is safe now that tahoe depends upon Twisted>=10.1, since notifyFinish
first appeared in Twisted-9.0

[nejucomo]

I see similar behavior, but after briefly skimming the patches and assuming that filenode.py was specific to only some HTTP requests, I filed a new ticket: #1664