Skip to content

This re-applies Gábor's stream closing work with some additional fixes#84

Closed
snorp wants to merge 6 commits into
molnarg:masterfrom
snorp:fixes
Closed

This re-applies Gábor's stream closing work with some additional fixes#84
snorp wants to merge 6 commits into
molnarg:masterfrom
snorp:fixes

Conversation

@snorp
Copy link
Copy Markdown
Contributor

@snorp snorp commented Aug 28, 2014

No description provided.

@snorp
Copy link
Copy Markdown
Contributor Author

snorp commented Aug 29, 2014

This still isn't correct wrt to the 'close' event in messages. That should only be emitted when the stream closes unexpectedly. Right now I think it fires any time the stream ends.

@snorp
Copy link
Copy Markdown
Contributor Author

snorp commented Aug 29, 2014

Also, not sure what's up with the 0.11 failures. I don't have that installed here, so I'll try to do that and look into it.

This fixes things so we emit 'close' only when it's an unexpected thing. Otherwise
'end' or 'finish' indicates normal termination.

See http://nodejs.org/api/http.html#http_event_close_1
as well as http://nodejs.org/api/http.html#http_event_close_2
@molnarg
Copy link
Copy Markdown
Owner

molnarg commented Sep 7, 2014

Thanks, I'll try to take care of the 0.11 fixes. There are subtle differences in the Stream class implementation which may cause trouble.

Also, the definition of the "close" event is ambigous as it means different things on different kind of streams. It does not always mean unexpected close (sorry for not providing references to the documentation, but it should be easy to find the different definitions of the close event).

For now, I merged this as close-stream-3 branch to not break CI, and will develop it there.

@molnarg molnarg closed this Sep 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants