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

emit event 'ended' on pool drained #571

Merged
merged 4 commits into from
Apr 22, 2014

Conversation

rngadam
Copy link

@rngadam rngadam commented Apr 17, 2014

I've noticed that there's no real way that I could see to be notified that all the pools have actually destroyed all their resources successfully. pg.end() ends just orders the pool to drain and destroyAllNow, but doesn't notify anyone when these actions are completed.

This change lets us do this with a new event "ended":

    var deferred = Q.defer();
    pg.on('ended', function() {
      deferred.resolve();
    });
    pg.on('error', function(err) {
      deferred.reject(err);
    });
    pg.end();
    return deferred.promise;

@brianc
Copy link
Owner

brianc commented Apr 18, 2014

Thanks! Very good idea. One question: why not use the more standard pg.emit('end') instead of ended? I like event names to be present tense - gives more sense of action in the code, and kinda follows node convention better.

@rngadam
Copy link
Author

rngadam commented Apr 18, 2014

I'm just not very good at naming! updated!

@brianc
Copy link
Owner

brianc commented Apr 22, 2014

Perfecto!

By the way - sorry for the ghetto state of the tests here. I have quite a few, but they were written before things like mocha or node-tap or anything existed, so they're a bit home-grown. I really appreciate you wading through them.

I'll merge & release asap.

brianc added a commit that referenced this pull request Apr 22, 2014
@brianc brianc merged commit 1047aeb into brianc:master Apr 22, 2014
@rngadam
Copy link
Author

rngadam commented Apr 23, 2014

Thanks for merging. I think the tests are pretty good in general, I'm happy that there's good coverage and I'm surprised I can usually find pretty quickly a test to model on when adding my own tests... How do you handle documentation? Do you write it up as part of the release?

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

Successfully merging this pull request may close these issues.

2 participants