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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(withIdPrefix): EventEmitter is never garbage collected #167

Merged
merged 5 commits into from Jul 23, 2018

Conversation

@Terreii
Copy link
Contributor

Terreii commented Jul 14, 2018

馃悶 The bug description

When store.withIdPrefix('prefix/') is called, an event handler is added to the parent/root EventEmitter. But there is, at the moment, no code to remove this event handler!
Which results in a memory leek: Every time the withIdPrefix-API is destroyed, its event handler is never removed!

store.withIdPrefix('test/').findAll() // The event handler, created here, is never removed!
  .then(doSomeThing)

馃搵 Steps to reproduce the behavior

  • Create with withIdPrefix() an instance of its API
  • delete this instance
  • Set a break point at with-id-prefix.js line 36
  • make a change event happen

鉁旓笍 Expected behavior

If the instance of the withIdPrefix-API is deleted, its event handler shouldn't be called. Or earlier.

馃挕 Solution

withIdPrefix() shouldn't add an event handler to the parent EventEmitter, but as soon as an event is added, the API instance will add its event handler. And the API instance will remove its event handler, as soon as no event handler is listening to it.

馃敡 Changes proposed in this pull request

Every time a event is added or removed, all active event handlers, on the EventEmitter of this withIdPrefix-API instance, will be checked.

  • If 0 handlers are listening: the handler will be removed from the parent EventEmitter
  • If >= 1 handlers are listening: the handler will be added to the parent EventEmitter

馃敟 BREAKING CHANGE: The Order of event-handler calling may change.
Because the handler of an instance of the withIdPrefix-API, is only added, if a handler is listening, the order of calling the handlers might change with this pull request!

// before
var devsStore = store.withIdPrefix('developers/')

store.on('change', calledSecond) // called last

devsStore.on('change', calledFirst) // called first

// now
var devsStore = store.withIdPrefix('developers/')

store.on('change', calledFirst) // called first

devsStore.on('change', calledSecond) // called last

鉀猴笍 馃惗 馃惢
馃 馃敟 馃悢

Terreii added 3 commits Jul 14, 2018
The withPrefix-API should only listen to events, if itself has event-handlers listening. This makes it posible for the withPrefix-API to be garbage collected.

BREAKING CHANGE: Order of event-handler calling may change.
// newListener event will be emitted before the listener will be added!
// But the removeListener event after the listener was removed!
// That is why wasAdded is needed.
var shouldListen = wasAdded || [

This comment has been minimized.

Copy link
@Terreii

Terreii Jul 14, 2018

Author Contributor

I properly should pass the eventName if it was added (and not wasAdded), and check it in the .some-function count > 0 || type === eventName.
This would make sure, that if a unknown event type was added, it wouldn't count.

Terreii added 2 commits Jul 14, 2018
If an event is added, only 'add', 'update', 'remove' and 'change' should be counted and checkt.
@janl
janl approved these changes Jul 23, 2018
Copy link
Member

janl left a comment

ace

@janl janl merged commit 8fcbdae into hoodiehq:master Jul 23, 2018
3 checks passed
3 checks passed
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@Terreii

This comment has been minimized.

Copy link
Contributor Author

Terreii commented Jul 23, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.