Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Fix EventEmitter.once issue and add EventEmitter removeAllListeners patch. #500

Merged
merged 7 commits into from
Nov 16, 2016

Conversation

JiaLiPassion
Copy link
Collaborator

@JiaLiPassion JiaLiPassion commented Nov 4, 2016

I found several issues of patching EventEmitter's listener related methods.

with zone.js 0.6.22 release, addListener, prependListener, removeListener are patched in this commit.
100b82b

  1. issue Zone.js higher than 0.6.21 breaks angular-universal https calls #484 EventEmitter.once was not removed properly.
    the issue can be described as the following test case.

emitter.once('test', shouldNotRun); emitter.removeListener('test', shouldNotRun);

in 0.6.22, the once listener was not removed.

the reason is when removeListener call util.ts, findExistingRegisteredTask method, it will look for the correct event task by check the eventTask.data.handler === handler, but if the handler is attached by EventEmitter.once method, the handler will not attached directly, instead it will be attached through a _onceWrap (https://github.com/nodejs/node/blob/master/lib/events.js#L286), and keep the original handler to the onceWrap.listener property, so we should not only check the data.handler but also the data.handler.listener(2ee4711#diff-2e7dc744ee86a31cde46d41cda5e42c0R714)

  1. issue Error: Can't set headers after they are sent. #491 removeListeners was not patched
    the issue can be described as the following test case.
    emitter.on('test', expectZoneA); emitter.on('test', expectZoneA); emitter.removeAllListeners('test');

in 0.6.22, the listener was not removed.

the reason is based on the issue #491, in the zone.js 0.6.22 release, to fix #430, addListener, removeListener, prependListener, and listeners have been patched, but removeAllListeners is not, In issue #491, socket-io will call removeAllListeners to cache and remove the listeners on request event when attach server, but because the removeAllListeners is not patched and handled correctly by zone(the response listener was added to event tasks because the addListener is patched but not removed because removeAllListeners are not patched), the request event will be handled multiple times and cause express response throw "can't set header after response sent " error.
I add a patch of removeAllListeners based on the #430 release.

  1. when add listener by prependListener, the listener should be added at the beginning of the listener array.
    the issue can be described as the following test case.

emitter.on('test', listenerA); emitter.prependListener('test', listenerB); expect(emitter.listeners('test')).toEqual([listenerB, listenerA]);

in 0.6.22, the order is still listenerA, listenerB

The reason is in 0.6.22 prependListener do the same logic with addListener.

  1. use the newest fetch to fix Object # has no method 'blob' error

please review the source code of those 2 issues, Thank you.

# Conflicts:
#	dist/long-stack-trace-zone.js
#	dist/long-stack-trace-zone.min.js
#	lib/zone-spec/long-stack-trace.ts
#	test/zone-spec/long-stack-trace-zone.spec.ts
@JiaLiPassion JiaLiPassion changed the title Add removeAllListeners patch to handle EventEmitter removeAllListeners Fix EventEmitter.once issue and add EventEmitter removeAllListeners patch. Nov 5, 2016
@jtmalinowski
Copy link
Contributor

Nice! I might be mistaken, but you should not commit dist versions, they are updated, when publishing a new version, right?

@JiaLiPassion
Copy link
Collaborator Author

@JakubMal Yes, you are right, I will modify my Pull Request. Thank you.

@JiaLiPassion
Copy link
Collaborator Author

I modify the package.json to use the newest fetch to avoid travis CI build error(Object # has no method 'blob').

@mhevery mhevery merged commit eff3353 into angular:master Nov 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants