Correctly handle long sleep times #40

Merged
merged 2 commits into from Jul 4, 2012

Conversation

Projects
None yet
3 participants

calmh commented Jul 1, 2012

Currently the code tries to setTimeout for too long intervals when
handling for example a monthly job. This commit fixes the wait logic so
that it doesn't try to sleep longer than the setTimeout maximum of
2^31-1 milliseconds (24.9 days).

@calmh calmh Correctly handle long sleep times
Currently the code tries to setTimeout for too long intervals when
handling for example a monthly job. This commit fixes the wait logic so
that it doesn't try to sleep longer than the setTimeout maximum of
2^31-1 milliseconds (24.9 days).
355d5cf

This pull request passes (merged 355d5cf into 6cf764b).

Collaborator

ncb000gt commented Jul 3, 2012

Can you add some tests. Once those are in I'll merge this in.

@calmh calmh Add test case for long wait
When scheduling a job 32 days into the future, we should not get the
callback within 250 ms.
cca07d9

calmh commented Jul 3, 2012

Added a test for the long wait case. In the unpatched version, the callback fires immediately and the test fails. In the patched version it succeeds. Verifying that it actually fires 31 days later is trickier. I guess you could mock out setTimeout and verify the calls to it, but it feels overkill under the circumstances.

This pull request passes (merged cca07d9 into 6cf764b).

Collaborator

ncb000gt commented Jul 4, 2012

I'm going to merge this with the addition of logging to inform anyone who tries to use a larger than supported timeout.

@ncb000gt ncb000gt added a commit that referenced this pull request Jul 4, 2012

@ncb000gt ncb000gt Merge pull request #40 from calmh/fix-long-sleep
Correctly handle long sleep times
7c7506e

@ncb000gt ncb000gt merged commit 7c7506e into kelektiv:master Jul 4, 2012

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