Opened 3 years ago

Closed 3 years ago

#5043 enhancement closed wontfix (wontfix)

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

Reported by: psykidellic Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch:
Author: Launchpad Bug:

Description

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 psykidellic 3 years ago.
Patch

Download all attachments as: .zip

Change History (5)

comment:1 Changed 3 years ago by psykidellic

  • 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 3 years ago by psykidellic

Patch

comment:2 Changed 3 years ago by exarkun

  • Component changed from core to web
  • Type changed from defect to enhancement

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 3 years ago by jonathanj

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 3 years ago by exarkun

  • Keywords review removed
  • Resolution set to wontfix
  • Status changed from new to closed

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.