leviwheatcroft / superagent-throttle

A plugin for superagent that throttles requests.
58 stars 17 forks source link

Uncaught, unspecified "error" event. #10

Closed polizz closed 7 years ago

polizz commented 7 years ago

I'm running into an issue that is reproducible, but over a wide range of simple GET requests. Some times it dies at 90'ish requests, other times at several thousand. It also happens over a wide range of throttle settings, from 10's per second to 1 per second.

The highest throughput I've seen before the error occurs is with these settings:

let throttle = new Throttle({
  active: true, // set false to pause queue
  rate: 20, // how many requests can be sent every `ratePer`
  ratePer: 1000, // number of ms in which `rate` requests may be sent
  concurrent: 10 // how many requests can be sent concurrently
})

Error:

events.js:168
      throw err;
Error: Uncaught, unspecified "error" event. ([object Object])
    at Throttle.emit (events.js:166:17)
    at cleanup (/Users/aw/projects/curbside/node_modules/superagent-throttle/dist/index.js:217:18)
    at Request.callback (/Users/aw/projects/curbside/node_modules/superagent/lib/node/index.js:688:3)
    at IncomingMessage.<anonymous> (/Users/aw/projects/curbside/node_modules/superagent/lib/node/index.js:883:18)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:188:7)
    at endReadableNT (_stream_readable.js:975:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickCallback (internal/process/next_tick.js:104:9)

Let me know how I can assist in troubleshooting it further.

polizz commented 7 years ago

I think I've figured it out. PR incoming for your review, if you are interested.

leviwheatcroft commented 7 years ago

Hey thanks very much for taking the time to look into this.. definitely interested to see what you come up with.

Error is thrown from here. Looking into EventEmitter docs it says emitting an 'error' event is some kind of special case. It doesn't explicitly say so but a few examples I've seen always emit an actual error object.

Is it possible that because the code here simple emits the err value passed in (not an instance of Error) the EventEmitter is throwing an error, but says it's unspecified ([object Object]) because the error message doesn't have the usual properties which describe an error (like the message property)?

As an aside.. I've always found it a bit confusing trying to discerne between http errors and, well.. errors ? For example, [this test]() ensures timeout errors are dealt with correctly, but I guess they're not http errors which are passed to the callback's err param.

Anyhow, L217 linked above was intended merely as something to subscribe to, rather than a definitive way to deal with errors. If my assumption above is correct, then emitting an 'error' event is crashing the process, where it should really just clean up the throttle and pass the err value back to the passed in callback. So basically, I think that if statement and emit call should simple be omitted.

I won't have time to look at this today, but definitely keen to resolve, would love to get a PR from you if you're interested. Thanks again.

leviwheatcroft commented 7 years ago

resolved

webuniverseio commented 7 years ago

I'm getting this error in browser. It seems that browser/index.js does not include fixes.

webuniverseio commented 7 years ago

Once that change is included in browser/index.js things start working properly.

leviwheatcroft commented 7 years ago

sorry, thanks very much for looking into it for me.

I've rebuilt and published 0.2.5 which includes this patch in all built resources