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

Fix concurrent callback calls #7

Merged
merged 1 commit into from
Nov 2, 2014
Merged

Conversation

ddfisher
Copy link

@ddfisher ddfisher commented Nov 2, 2014

Currently, gulp-batch will call the callback more than once concurrently if it gets an event while running the callback. This is because the module-level holdOn variable is not correctly set to true (the var holdOn = ... should just be holdOn = ...)!

Only removing the extraneous var would cause gulp-batch to completely ignore events while the callback is running, which is undesirable. This patch stores events that occur during the callback, and - if any occurred - waits for the timeout interval after the callback ends and flushes them.

floatdrop added a commit that referenced this pull request Nov 2, 2014
Fix concurrent callback calls
@floatdrop floatdrop merged commit 44913d6 into floatdrop:master Nov 2, 2014
@floatdrop
Copy link
Owner

Thanks! Published in 1.0.2

@UltCombo
Copy link
Contributor

UltCombo commented Nov 2, 2014

@floatdrop I've pulled gulp-batch's latest master to gulp-watch's dependencies, and gulp-watch's callback is only ever firing once now. Can you check this?

Make a simple gulp-watch task, then edit watched files twice or more. It logs the "was changed" message but does not fire the callback from the second time onwards.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 2, 2014

@floatdrop given the current unit test that is failing, I'd guess asyncDone's callback is never fired.

@floatdrop
Copy link
Owner

@UltCombo I bet it because of you don't call callback or return Stream/Promise.

@UltCombo
Copy link
Contributor

UltCombo commented Nov 2, 2014

@floatdrop damn GitHub didn't show your comment automatically, and you're right. I had to dig it in the async-done docs:

Warning: Sync taks are not supported and your function will never complete if the one of the above strategies is not used to signal completion. However, thrown errors will be caught by the domain.

@ddfisher
Copy link
Author

ddfisher commented Nov 4, 2014

Thanks for the quick merge! :)

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.

None yet

3 participants