Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DeltaManagerProxy causes troubles when emitting "error" events. #33

Closed
vladsud opened this issue Sep 17, 2019 · 1 comment · Fixed by #57
Closed

DeltaManagerProxy causes troubles when emitting "error" events. #33

vladsud opened this issue Sep 17, 2019 · 1 comment · Fixed by #57
Assignees

Comments

@vladsud
Copy link
Contributor

vladsud commented Sep 17, 2019

This stack is problematic:
at DeltaManagerProxy.emit (events.js:132)
at DeltaManager. (deltaManagerProxy.ts:23)
at DeltaManager.emit (events.js:151)
at DeltaQueue. (deltaManager.ts:185)
at DeltaQueue.emit (events.js:151)
at callback (deltaQueue.ts:137)
at DeltaQueue.worker (deltaManager.ts:176)
at DeltaQueue.processDeltas (deltaQueue.ts:151)

Note EventEmitter implementation - this screws up error processing - we should not throw from here (but we do because of DeltaManagerProxy):

// If there is no 'error' event listener then throw.
if (doError) {
var er;
if (args.length > 0)
er = args[0];
if (er instanceof Error) {
// Note: The comments on the throw lines are intentional, they show
// up in Node's output if this results in an unhandled exception.
throw er; // Unhandled 'error' event
}

@vladsud vladsud changed the title DeltaManagerProxy causes troubles when emitting "erorr" events. DeltaManagerProxy causes troubles when emitting "error" events. Sep 17, 2019
@arinwt
Copy link
Contributor

arinwt commented Sep 17, 2019

This issue was discovered in 0.9, before DeltaManagerProxy was set to use the EventForwarder base class (introduced in this PR). This should be mostly resolved in 0.10+, because the EventForwarder does not subscribe to the source emitter events until someone listens to the forwarder for that event.

Problem (in 0.9):

  1. someone meaningfully listens to DeltaManager error event.
  2. DeltaManagerProxy listens to DeltaManager error event.
  3. nobody listens to DeltaManagerProxy error event.
  4. DeltaManager raises error event, calling all listeners.
  5. DeltaManagerProxy throws error because it has no error listeners.

Same flow with EventForwarder, no problem (in 0.10):

  1. someone meaningfully listens to DeltaManager error event.
  2. nobody listens to DeltaManagerProxy error event.
  3. because of (2), DeltaManagerProxy does not listen to DeltaManager error event.
  4. DeltaManager raises error event, calling all listeners.

But it could still happen with an unsubscribe (in 0.10):

  1. someone meaningfully listens to DeltaManager error event.
  2. someone listens to DeltaManagerProxy error event.
  3. because of (2), DeltaManagerProxy listens to DeltaManager error event.
  4. someone removes their only listener to DeltaManagerProxy error event.
  5. DeltaManager raises error event, calling all listeners.
  6. DeltaManagerProxy throws error because it has no error listeners.

I doubt this occurs in practice, because the proxy will likely be disposed which will disconnect it from the source emitter, but should still be fixed. When all listeners to a single event are removed from the proxy, it should stop listening to the source emitter for that event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants