max-mapper / websocket-stream

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

Error event emits without error object #138

Open binarykitchen opened 6 years ago

binarykitchen commented 6 years ago

In my videomail client code this strange thing sometimes happens: no error object is emited along with the error event.

Like

stream.on('error', function (err) {
  console.log(err) // this will print undefined
})

Don't you agree, it should have an error object? An error without an error is not an error :)

binarykitchen commented 6 years ago

Btw in the stack trace I can see this happens when queue is drained and Duplexify._destroy is called.

mcollina commented 6 years ago

Can you include that stacktrace?

binarykitchen commented 6 years ago

i am afraid, due to wrong configs, browserify sourcemaps are broken on production, hence this stack trace is kinda cryptic and not very useful:

Error
    at Function.VideomailError.create (https://videomail.io/js/main-1000576986.min.js:3:906305)
    at null.<anonymous> (https://videomail.io/js/main-1000576986.min.js:3:971808)
    at https://videomail.io/js/main-1000576986.min.js:3:735480
    at EventEmitter.emit (https://videomail.io/js/main-1000576986.min.js:3:735605)
    at Duplexify._destroy (https://videomail.io/js/main-1000576986.min.js:3:727187)
    at https://videomail.io/js/main-1000576986.min.js:3:727047
    at Item.run (https://videomail.io/js/main-1000576986.min.js:3:778418)
    at drainQueue (https://videomail.io/js/main-1000576986.min.js:3:777238)
    at arlong:beforeload:6:321
mcollina commented 6 years ago

Let us know if you can reproduce this reliably and/or have a better stacktrace.

binarykitchen commented 6 years ago

I'm afraid very hard to reproduce. Happens by random on a heavy traffic site.

Do you have a guess what this could be?

mcollina commented 6 years ago

Not really. Did it happen with the latest version of readable-stream? Can you try to roll that back?

binarykitchen commented 6 years ago

how? not using readable-stream anywhere in the videomail-client code

mcollina commented 6 years ago

Duplexify is relying on readable-stream internally.

binarykitchen commented 6 years ago

do you recommend to roll back readable-stream by rolling back websocket-stream? cos it's a nested-nested dependency

mcollina commented 6 years ago

Did this happen after a very recent deploy? I released a new version a week ago.

binarykitchen commented 6 years ago

no, happened before as well

mcollina commented 6 years ago

then no, that's not the problem. We really need a stacktrace and/or a good way to reproduce.

binarykitchen commented 6 years ago

ugh, this will be difficult for me.

is there a useful tool/package to inspect/record a stream of all its events/data it had till the error was emitted?

Juul commented 6 years ago

As far as I can tell websocket-stream doesn't originate any errors itself but does propagate errors from the underlying WebSocket which of course is just a plain TCP socket, so your null errors could be either coming from the ws module or from the browser or from the operating system's socket implementation.

binarykitchen commented 6 years ago

@Juul @mcollina ok now i always can reproduce this. and it is on iphones only:

  1. open www.videomail.io on safari
  2. allow camera, initiate connection blahblah
  3. hit the sleep button on your iphone
  4. let it sleep for at least one min
  5. unlock and it should reopen same site on safari
  6. now you see that weird error

i expect websocket-stream to resume but not to throw an error. or at least throw a descriptive error.

mcollina commented 6 years ago

I think this is a safari-mobile specific bug. If you can get a stacktrace, we can definitely generate a new error if it's not provided by the platform.

binarykitchen commented 6 years ago

hard to get a stack trace here i am afraid - do you have any other examples running online producing valid stacktraces i could test from my iphone?

mcollina commented 6 years ago

You should be able to use Remote Web Inspector.

binarykitchen commented 6 years ago

good idea. turns out it is an event object, not an error object itself.

this screenshot tells more:

screen shot 2018-02-28 at 13 21 40

when debugging deep down, looks like

  function onerror(err) {
    stream.destroy(err)
  }

is the starting point. and funny is that this references to the global Window where this.destroyed is undefined. i expect it to be destroyed when iphone is locked/sleeping.

mcollina commented 6 years ago

@binarykitchen let us know if you can send a fix!

binarykitchen commented 6 years ago

dont know if i can.

socket.onerror <-- what instance is socket here in the browser?

its implementation has to be questioned here.

mcollina commented 6 years ago

it's a WebSocket

binarykitchen commented 6 years ago

native implementation?

mcollina commented 6 years ago

the WebSocket from the browser.

binarykitchen commented 6 years ago

okay, sounds like it's an ios bug of its websocket implementation ... where to start best?

mcollina commented 6 years ago

You should probably work around that in your code.

binarykitchen commented 6 years ago

already have but then, what if other non-ios browsers truly emit an error instead of an event during stream errors? then my app code gets complicated just because of ios weird websocket error handling?

mcollina commented 6 years ago

unfortunately yes.

binarykitchen commented 6 years ago

alright :(

... btw have reported to apple as well and will keep you posted here.