node js Stream must implement destroy #79

Closed
dominictarr opened this Issue Jul 6, 2012 · 9 comments

Comments

Projects
None yet
4 participants
@dominictarr

Since the server side of sockjs is a stream, it needs to implement destroy
as destroy is called by pipe in some cases.

https://github.com/joyent/node/blob/master/lib/stream.js#L74

It should basically do what close does, but without asking.
It should just terminate any connection... which will mean calling destroy
on whatever underling stream is currently in use.

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Jul 6, 2012

oh, oops, I see do implement destroy.
but not correctly.
you should not removeAllListeners

destroy should set this.writable = false
dispose of any resources, and then emit 'close'

indeed, pipe, will listen on 'close' and calling this.removeAllListeners() will leave pipe hanging.

I am working on a guide for correct streams.
https://github.com/dominictarr/stream-spec/blob/master/stream_spec.md

it should be helpful.

oh, oops, I see do implement destroy.
but not correctly.
you should not removeAllListeners

destroy should set this.writable = false
dispose of any resources, and then emit 'close'

indeed, pipe, will listen on 'close' and calling this.removeAllListeners() will leave pipe hanging.

I am working on a guide for correct streams.
https://github.com/dominictarr/stream-spec/blob/master/stream_spec.md

it should be helpful.

@majek

This comment has been minimized.

Show comment
Hide comment
@majek

majek Jul 6, 2012

Member

Yes, destroy is implemented already:
https://github.com/sockjs/sockjs-node/blob/dev/src/transport.coffee#L47

Correct me if I'm wrong about removeAllListeners but node docs say:

http://nodejs.org/api/stream.html#stream_stream_destroy

stream.destroy()#
Closes the underlying file descriptor. Stream will not emit any more events.
Member

majek commented Jul 6, 2012

Yes, destroy is implemented already:
https://github.com/sockjs/sockjs-node/blob/dev/src/transport.coffee#L47

Correct me if I'm wrong about removeAllListeners but node docs say:

http://nodejs.org/api/stream.html#stream_stream_destroy

stream.destroy()#
Closes the underlying file descriptor. Stream will not emit any more events.
@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Jul 7, 2012

oh, thats a mistake in the documentation. I'll look into getting that fixed.

it should be "no more 'data', 'end', or 'drain' events will be emitted"

look here, in fs

https://github.com/joyent/node/blob/master/lib/fs.js#L1359-1383

and here in net

https://github.com/joyent/node/blob/master/lib/net.js#L322-374

destroy causes either 'close' or 'error' to be emmited.

oh, thats a mistake in the documentation. I'll look into getting that fixed.

it should be "no more 'data', 'end', or 'drain' events will be emitted"

look here, in fs

https://github.com/joyent/node/blob/master/lib/fs.js#L1359-1383

and here in net

https://github.com/joyent/node/blob/master/lib/net.js#L322-374

destroy causes either 'close' or 'error' to be emmited.

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Jul 11, 2012

my pull request has been merged, and the documentation is now corrected.
http://nodejs.org/api/stream.html#stream_event_close

my pull request has been merged, and the documentation is now corrected.
http://nodejs.org/api/stream.html#stream_event_close

@majek

This comment has been minimized.

Show comment
Hide comment
@majek

majek Jul 12, 2012

Member

Great! Destroy definitely should be modified to adhere to the new docs. Now the ball is on my side.

Member

majek commented Jul 12, 2012

Great! Destroy definitely should be modified to adhere to the new docs. Now the ball is on my side.

@ghost ghost assigned majek Jul 12, 2012

@theyak

This comment has been minimized.

Show comment
Hide comment
@theyak

theyak Oct 4, 2012

Any updates on this?

theyak commented Oct 4, 2012

Any updates on this?

@brycekahle

This comment has been minimized.

Show comment
Hide comment
@brycekahle

brycekahle Oct 22, 2014

Contributor

This applied to old streams, but now we should be using stream.Duplex

Contributor

brycekahle commented Oct 22, 2014

This applied to old streams, but now we should be using stream.Duplex

@brycekahle

This comment has been minimized.

Show comment
Hide comment
@brycekahle

brycekahle Oct 22, 2014

Contributor

@dominictarr Do you think my change is sufficient to fix this issue?

Contributor

brycekahle commented Oct 22, 2014

@dominictarr Do you think my change is sufficient to fix this issue?

@brycekahle

This comment has been minimized.

Show comment
Hide comment
@brycekahle

brycekahle Oct 28, 2014

Contributor

I think this is fixed by my commit. Reopen if you think otherwise.

Contributor

brycekahle commented Oct 28, 2014

I think this is fixed by my commit. Reopen if you think otherwise.

@brycekahle brycekahle closed this Oct 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment