Stop the watchFileChecker callback from being called twice (closes #110) #113

Merged
merged 0 commits into from Jul 5, 2012

Conversation

4 participants
@CoderPuppy

See the title and commits.

@remy

This comment has been minimized.

Show comment Hide comment
@remy

remy Jul 5, 2012

Owner

@dylanmcd can I get your eyes on this pull request - it's inside your update to nodemon and truth be told, I'm losing track a little! :)

Owner

remy commented Jul 5, 2012

@dylanmcd can I get your eyes on this pull request - it's inside your update to nodemon and truth be told, I'm losing track a little! :)

@remy

This comment has been minimized.

Show comment Hide comment
@remy

remy Jul 5, 2012

Owner

@drewyoung1 code could do with a comment or two - I get the "fix app starting multiple times" - but how?

Owner

remy commented Jul 5, 2012

@drewyoung1 code could do with a comment or two - I get the "fix app starting multiple times" - but how?

@remy remy referenced this pull request Jul 5, 2012

Closed

Express with Nodemon #110

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

Ok

Ok

@dylanmcd

This comment has been minimized.

Show comment Hide comment
@dylanmcd

dylanmcd Jul 5, 2012

Contributor

@remy @drewyoung1 That solution is fine, but I think a simpler way would be to do.

fs.watch(watchFileName, function(event, filename) {
  if (watchFileChecker.changeDetected) { return; }
  watchFileChecker.changeDetected = true;
  cb(true);
});

If changeDetected has been toggled, we know the callback has been called already. Only requires one added line, if (watchFileChecker.changeDetected) { return; }. Is that ok with you drew?

Contributor

dylanmcd commented Jul 5, 2012

@remy @drewyoung1 That solution is fine, but I think a simpler way would be to do.

fs.watch(watchFileName, function(event, filename) {
  if (watchFileChecker.changeDetected) { return; }
  watchFileChecker.changeDetected = true;
  cb(true);
});

If changeDetected has been toggled, we know the callback has been called already. Only requires one added line, if (watchFileChecker.changeDetected) { return; }. Is that ok with you drew?

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

But watchFileChecker.verify also calls cb

But watchFileChecker.verify also calls cb

@dylanmcd

This comment has been minimized.

Show comment Hide comment
@dylanmcd

dylanmcd Jul 5, 2012

Contributor

Only if changeDetected is false, and if it's false, that means that the fs.watch callback never got called. What's going on here is a test to see if fs.watch works. If it does, changeDetected will get toggled to true. If it doesn't, then when the timeout fires, verify will see that changeDetected hasn't been toggled to true, and will call the callback with false, letting the program know that fs.watch isn't supported. There cannot be a circumstance where both verify and fs.watch callbacks are both triggered.

The memory leak seems to be caused by fs.watch triggering more than once for some reason.

Contributor

dylanmcd commented Jul 5, 2012

Only if changeDetected is false, and if it's false, that means that the fs.watch callback never got called. What's going on here is a test to see if fs.watch works. If it does, changeDetected will get toggled to true. If it doesn't, then when the timeout fires, verify will see that changeDetected hasn't been toggled to true, and will call the callback with false, letting the program know that fs.watch isn't supported. There cannot be a circumstance where both verify and fs.watch callbacks are both triggered.

The memory leak seems to be caused by fs.watch triggering more than once for some reason.

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

Ok

Ok

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

Better?

Better?

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

Oh. I don't think that's what you were talking about.
Ok I'll do it in the fs.watch callback.

Oh. I don't think that's what you were talking about.
Ok I'll do it in the fs.watch callback.

@CoderPuppy CoderPuppy merged commit 08e5aa1 into remy:master Jul 5, 2012

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

So how did that get merged the instant I pushed???

So how did that get merged the instant I pushed???

@dylanmcd

This comment has been minimized.

Show comment Hide comment
@dylanmcd

dylanmcd Jul 5, 2012

Contributor

I don't know, doesn't look like it actually got merged...want me to just do the one line patch through the git interface?

Contributor

dylanmcd commented Jul 5, 2012

I don't know, doesn't look like it actually got merged...want me to just do the one line patch through the git interface?

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

Ok

Ok

@dylanmcd

This comment has been minimized.

Show comment Hide comment
@dylanmcd

dylanmcd Jul 5, 2012

Contributor

Can you test that @drewyoung1 and make sure it's working, as I can't reproduce it here. Thanks for your help on this.

Contributor

dylanmcd commented Jul 5, 2012

Can you test that @drewyoung1 and make sure it's working, as I can't reproduce it here. Thanks for your help on this.

@CoderPuppy

This comment has been minimized.

Show comment Hide comment
@CoderPuppy

CoderPuppy Jul 5, 2012

Yep

Yep

@bluekeys

This comment has been minimized.

Show comment Hide comment
@bluekeys

bluekeys Jul 7, 2012

It seems this fix somehow hasn't made it into the repo yet? I was suffering the same problem, copy pasted the one liner and all seems well.

bluekeys commented Jul 7, 2012

It seems this fix somehow hasn't made it into the repo yet? I was suffering the same problem, copy pasted the one liner and all seems well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment