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

Clean up timers when the cache is stopped. #27

Merged
merged 1 commit into from Apr 16, 2015

Conversation

@jagoda
Copy link
Contributor

jagoda commented Mar 31, 2015

Pending timers cause the Node process to remain running even after the rest of the program has completed. This change causes all pending timers to be cleared when the cache service is stopped.

@jagoda jagoda force-pushed the jagoda:fix-clean-timers branch from ab33817 to c75034a Apr 4, 2015
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 4, 2015

Build looks happy now. Any thoughts on this?

@@ -69,6 +69,13 @@ internals.Connection.prototype.start = function (callback) {

internals.Connection.prototype.stop = function () {

// Clean up pending eviction timers.
for (var segment in this.cache) {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Apr 8, 2015

Contributor

Can you please change both of these for...in loops to iterate over Object.keys().

This comment has been minimized.

Copy link
@jagoda

jagoda Apr 9, 2015

Author Contributor

Sure, no problem. Should this be updated in the Hapi style guide?

child.on('exit', function () {

var stop = Date.now();
expect(stop - start).to.be.lessThan(500);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Apr 8, 2015

Contributor

It seems like this could potentially be flakey on a busy CI system. Can you come up with a different way to test this - preferably without needing to spawn a child process.

This comment has been minimized.

Copy link
@jagoda

jagoda Apr 9, 2015

Author Contributor

Agreed. I tried to avoid this the first time around, but didn't come up with anything. I have an idea that might work though -- will give it a try. Thanks for the extra push :-)

@jagoda jagoda force-pushed the jagoda:fix-clean-timers branch 2 times, most recently from bd31071 to 0736ccc Apr 9, 2015
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 9, 2015

I believe that I have addressed all concerns. Please let me know if I misunderstood anything.

@cjihrig cjihrig added the feature label Apr 13, 2015
@cjihrig cjihrig added this to the 1.1.2 milestone Apr 13, 2015
@cjihrig cjihrig self-assigned this Apr 13, 2015
@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Apr 13, 2015

After thinking about this a little bit more, would it be enough to just unref() the timer when it is initially created? It would be a one line code change to accomplish the same thing.

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 13, 2015

I considered this approach. However, the Node documentation seems to discourage it for performance reasons.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Apr 13, 2015

Fair enough

@@ -379,6 +381,54 @@ describe('Memory', function () {
done();
});

describe('stopping after set', function () {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Apr 13, 2015

Contributor

Let's do all of this in a single test. Also, set the test's parallel: false, as this isn't safe to run along other tests.

This comment has been minimized.

Copy link
@jagoda

jagoda Apr 14, 2015

Author Contributor

Sure, parallel : false makes total sense. I want to make sure that I understand your 'single test' point. By that do you mean a single it clause rather than a describe block with before and after? My concern with this approach is that in the event of a test failure the rest of the test cases could end up running without the setTimeout and clearTimeout changes being cleaned up. Given the current mock implementations, I don't expect there to be any noticable problems in this case, but allowing changes to the global state to possibly hang around seemed like a bad idea to me. Happy to do whatever you think is best though.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Apr 14, 2015

Contributor

Yes, a single it. Your method patches aren't significant. Just make sure to restore the old behavior before making any assertions. Once the test is fixed up, this should be good to go.

This comment has been minimized.

Copy link
@jagoda

jagoda Apr 14, 2015

Author Contributor

Fair enough.

@jagoda jagoda force-pushed the jagoda:fix-clean-timers branch from 0736ccc to b172494 Apr 14, 2015
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 14, 2015

I think that should do it. Let me know if I missed something.

@hueniverse hueniverse added bug and removed feature labels Apr 14, 2015
@@ -15,6 +15,8 @@ var internals = {};

var lab = exports.lab = Lab.script();
var describe = lab.describe;
var before = lab.before;

This comment has been minimized.

Copy link
@cjihrig

cjihrig Apr 16, 2015

Contributor

These two lines can be removed again.

This comment has been minimized.

Copy link
@jagoda

jagoda Apr 16, 2015

Author Contributor

Doh! Sorry about that.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Apr 16, 2015

One more change, then I'll land this.

@jagoda jagoda force-pushed the jagoda:fix-clean-timers branch from b172494 to b896342 Apr 16, 2015
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Apr 16, 2015

Done.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Apr 16, 2015

Thanks!

@cjihrig cjihrig closed this Apr 16, 2015
@cjihrig cjihrig reopened this Apr 16, 2015
cjihrig added a commit that referenced this pull request Apr 16, 2015
Clean up timers when the cache is stopped.
@cjihrig cjihrig merged commit e154997 into hapijs:master Apr 16, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jagoda jagoda deleted the jagoda:fix-clean-timers branch Apr 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.