Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Updated http library docs. #3012

Closed
wants to merge 2 commits into from

7 participants

@shaybenmoshe

Added the 'finish' event to 'http.ServerResponse' class.

@koichik
Owner

Ryan said:

'finish' is a private event and should not be used.

@shaybenmoshe

May I ask why 'finish' is a private event?
And is there any alternative event?

@bnoordhuis

May I ask why 'finish' is a private event?

It's for internal bookkeeping.

And is there any alternative event?

Depends. What are you trying to accomplish?

@shaybenmoshe

I want to be notified when 'res.end()' is executed successfully.

@bnoordhuis

I want to be notified when 'rep.end()' is executed successfully.

Doesn't the 'end' event do what you want?

@shaybenmoshe

Do you mean the event 'close'?
I can't find any event named 'end' for http.ServerResponse in the docs.

@bnoordhuis

Sorry for the delay, seems I never got a GH notification for this.

'end' is emitted when the message is complete. It's undocumented right now but I'll take a patch for that (I guess you can repurpose this one). Can you sign the CLA while you're at it? Thanks!

@shaybenmoshe

No problem, signed the CLA online.

@bnoordhuis

I guess I was wrong, end is an alias for close.

@isaacs, @koichik: It seems kind of silly that there is no good way to check for end of message except polling res.finished. Maybe we should publish the finish event (perhaps renamed). Thoughts?

@shaybenmoshe

I don't understand what is wrong with finish.
Any way, we can add 2 events, one fired before finish and the other after it.

@bnoordhuis

I don't understand what is wrong with finish

finish is an internal event; its meaning may change, it could get removed altogether, etc. If we decide to promote it to documented and supported API status, it needs to have a well-defined meaning and work in a way that's still acceptable two years from now. In other words, some thought should be put into it.

@shaybenmoshe

I understand, but I think we all agree that there should be an event that is emitted when we end the request.
There are so many use cases for this...

@isaacs
Owner

So, the issue is that you need to know when 'end' is called on a writable stream, or when the bytes are finished getting pushed out?

What's wrong with close?

I would not like to make the finish event public. We already have way too many things with subtly different meanings of "it's over". Adding another one is not the solution.

@bnoordhuis

I think we're mixing up things here (but it's quite possible that I'm the one doing the mixing. If that's the case, mea culpa.)

What's wrong with close?

'close' is emitted when the socket is closed, not when it's recycled for a new request. If you make two sequential requests to example.com:80, you cannot easily tell when the first request finishes - no event is emitted. That's where 'finish' comes in.

(It's kind of an awkward name. Maybe it should get renamed to 'end', that would be consistent with IncomingMessage.)

@bnoordhuis

@isaacs So what do you think?

@isaacs
Owner

I have no problem with documenting it. We may change this in the future, though.

@PureRumble

I can think of many use cases for wanting to know when the server has successfully fed the client with return data via http.ServerResponse, especially because I've implemented my own Server framework (sorry, but I didn't like express :-) ).

For instance, in my Server I wanna make sure the request has been served and successfully ended. Therefore it should be reported to the Server that the request has been served ONCE ServerResponse.end() has been called AND successfully ended the request.

Obviously I need a reliable event or some other way to attach a CB to the call of end().

So if this "finish" event really indicates the successful ending of the server's response to the client, then yes please BUMP IT UP to public documentation and keep it there :-)

Anyone else with thoughts on this?

@isaacs
Owner

@PureRumble While I definitely agree with you (and others in this thread) about the need for a way to know this, it's a bit more complicated.

In v0.9, we're going to revise the Stream API, and we don't want to have to do so ever again, and we want to minimize the impact of the revision. So, now is basically the worst possible time to be blessing new events and APIs.

@bnoordhuis

I agree with @isaacs. Let's keep the issue open but punt on it for now.

@shaybenmoshe

I agree with you two too.
However this issue should be kept in mind and not disregarded, quiet simple feature but an important one as well.

@shaybenmoshe

Please do something with that issue.
It really is important.

@kapouer

Just note that express 3 is using the finish event too...

@isaacs
Owner

The finish event is now public, and on all Writable stream objects. Fixed in master.

@isaacs isaacs closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 0 deletions.
  1. +6 −0 doc/api/http.markdown
View
6 doc/api/http.markdown
@@ -286,6 +286,12 @@ interface. This is an `EventEmitter` with the following events:
Indicates that the underlaying connection was terminated before
`response.end()` was called or able to flush.
+### Event: 'finish'
+
+`function () { }`
+
+Indicates that `response.end()` was called and executed properly.
+
### response.writeContinue()
Sends a HTTP/1.1 100 Continue message to the client, indicating that
Something went wrong with that request. Please try again.