search

pull-stream / stream-to-pull-stream

convert a node stream (classic or new) into a pull-stream
MIT License
30 stars 5 forks source link

Fix/destroyed read #14

Closed jacobheun closed 5 years ago

jacobheun commented 5 years ago

ended is already being set at the top of the read, so I removed the redundant assignment.

This adds an additional check of stream.writable before writing to the stream. This helps prevent some badly behaving streams from throwing an error when the stream has been destroyed, but ended has not been set.

I hit this with some connections using net sockets and this was able to prevent the writes.

dominictarr commented 5 years ago

this isn't quite right - if the node-stream has ended (because it's !writable) but the pull-stream source hasn't ended if(ended=null | !(writable=false)) then it will call done(null) but leave the source stream hanging.

instead it needs to abort the source stream in that case. but really, the node-stream should have emitted an error or close by then? If it emits an error or close later then this adapts it to a particular pattern of badly behaving stream, that does one thing wrong then another thing right. Yes, I think this needs to abort the source and call cleanup to unsubscribe from the stream's events.

jacobheun commented 5 years ago

Aborting makes sense, I've updated the code to abort and call cleanup.

I also noticed there are two destroy functions defined so only the second is getting called, as well as a first function that's never called. I didn't remove them to keep the change here small.

dominictarr commented 5 years ago

how are you testing that this works? this doesn't call done. I think this is gonna need a test case before I can merge it.

jacobheun commented 5 years ago

I'll work on a test case for this.

I was testing it against js-libp2p tests that are failing at https://github.com/libp2p/js-libp2p/pull/269. I'm also working on root fixes for that specific error, but also wanted to make stream-to-pull-stream more durable against future issues.

jacobheun commented 5 years ago

I've added a test, which did show the issue with done not being called, so that's now corrected.

jacobheun commented 5 years ago

I'm going to close this. I resolved the offending stream implementations. This might occur in the future but those streams should just get fixed.