Opened 7 years ago

Closed 6 years ago

#5043 enhancement closed wontfix (wontfix)

t.w.http.Request.finish should log error instead of raising RuntimeError

Reported by: RN Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch:


Calling Request.finish on a closed connection raises RuntimeError but its not forwarded to corresponding d.errback().

At this point of operation, there is nothing much anybody can do so it would be just better to log the message instead of raise an exception.

Attachments (1)

issue-5043.bugfix (2.3 KB) - added by RN 7 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 7 years ago by RN

Keywords: review added

Find attached a patch for same.

It applies to trunk at 31585. I have implemented the changes as mithrandi asked in bug 4901.

Changed 7 years ago by RN

Attachment: issue-5043.bugfix added


comment:2 Changed 7 years ago by Jean-Paul Calderone

Component: coreweb
Type: defectenhancement

At this point of operation, there is nothing much anybody can do

Woah, what? There is certainly a better thing you can do - don't call write.

This was introduced in #4317. Now that the feature is in a release - 11.0 - I wonder who wins by removing it from the next one. There's always the possibility that you'll get an exception if you write to a finished request

but its not forwarded to corresponding d.errback().

I don't follow this. What Deferred do you expect to errback when this situation is encountered? The exception is raised at application code, so application code can do whatever it wants with it. There's no notifyOnFinish Deferred to fire because the request already fired those - because this exception is only raised when the request is finished already.

As far as the patch goes, it seems like a fine implementation of the proposed change. I am unconvinced about the change, though.

comment:3 Changed 7 years ago by Jonathan Jacobs

Following the conversation from #4901, it looks as though the XML-RPC code should be using notifyFinish, instead of just calling Request.finish, to avoid the exception in the first place.

I don't think removing the exception that occurs when calling finish on a finished Deferred is something worth doing.

comment:4 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Resolution: wontfix
Status: newclosed

It seems like this has been sitting here for long enough. :) Going by the previous two comments, the bug here is actually invalid. #4901 should be resolved as jonathanj suggested, by tracking the request connection state and giving up early if necessary.

Note: See TracTickets for help on using tickets.