search

max-mapper / websocket-stream

websockets with the node stream API
BSD 2-Clause "Simplified" License
668 stars 114 forks source link

Delay writing in browsers until the buffer has drained #78

Closed jnordberg closed 8 years ago

jnordberg commented 8 years ago

As discussed in #1

mcollina commented 8 years ago

I'm :+1:. Not sure this can be easily tested, if you have some idea just add one :).

mcollina commented 8 years ago

Thinking a bit more about this, I fear we might need to put in a linear or exponential timeout in there, rather than a fixed 10ms (with a cap of 2-5s). If a user is on a slow connection, we'll be polling aggressively eating a lot of CPU power. When a user is on a slow connection, he is probably on battery, so better be nice. 10ms is too slow for a full round-trip anyway on the net.

jnordberg commented 8 years ago

Good point. I also noticed another thing now when using this: chrome throttles setTimeout's to max once a second when in a background tab, making background uploads super slow.

We could take the time and byte delta between two writes and try to figure out what the buffer size and delay should be... But maybe the best/simplest solution would be to just allow the socket buffer to grow to 10mb and set a fixed timeout of 1s? That would not allow for accurate speed measures for small files but at least it addresses the problem of cpu usage and background uploads.

mcollina commented 8 years ago

I'm :+1: on that solution. However we should make that delay configurable. A magic number is fine, but let's give users a choice.

jnordberg commented 8 years ago

Made both parameters configurable and came up with some defaults after some testing with my crappy internet connection. Can't test locally since the buffer drains way faster than i can fill it.

500kb buffer with a 1000 ms interval seems to be a good compromise

mcollina commented 8 years ago

I'm :+1: on this. Any other opinion: @mafintosh @maxogden?

max-mapper commented 8 years ago

just reviewed it, seems good to me. @mafintosh can you see any stream edge cases with this approach?

mafintosh commented 8 years ago

@maxogden no :+1: from me.

mcollina commented 8 years ago

LGTM Il giorno ven 11 dic 2015 alle 00:28 Mathias Buus notifications@github.com ha scritto:

@maxogden https://github.com/maxogden no [image: :+1:] from me.

— Reply to this email directly or view it on GitHub https://github.com/maxogden/websocket-stream/pull/78#issuecomment-163785433 .

jnordberg commented 8 years ago

Added some documentation

max-mapper commented 8 years ago

@jnordberg awesomeness! I just published it as 2.3.0. I also added you as a collaborator (open open source style)

jnordberg commented 8 years ago

Cool! I'll make sure to follow the open open source spirit :) Happy holidays!