Page MenuHomePhabricator

make Trigger Daemon sleep correctly when one-time triggers exist
ClosedPublic

Authored by avivey on Apr 18 2016, 8:51 PM.
Tags
None
Referenced Files
F14844317: D15750.id37955.diff
Sun, Feb 2, 2:58 PM
Unknown Object (File)
Mon, Jan 27, 12:00 PM
Unknown Object (File)
Fri, Jan 24, 1:20 PM
Unknown Object (File)
Fri, Jan 24, 1:20 PM
Unknown Object (File)
Fri, Jan 24, 1:20 PM
Unknown Object (File)
Fri, Jan 24, 1:20 PM
Unknown Object (File)
Tue, Jan 21, 12:42 PM
Unknown Object (File)
Mon, Jan 13, 5:28 PM
Subscribers

Details

Summary

Trigger daemon is trying to find the next event to invoke before sleeping, but the query includes already-elapsed triggers.
It then tries to sleep for 0 seconds.

Test Plan

On a new instance, schedule a single trigger of type PhabricatorOneTimeTriggerClock to a very near time.

Use top to see trigger daemon not going to 100% CPU once the event has elapsed.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 11827
Build 14839: Run Core Tests
Build 14838: arc lint + arc unit

Event Timeline

avivey retitled this revision from to make Trigger Daemon sleep correctly when one-time triggers exist.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a reviewer: epriestley.
epriestley edited edge metadata.
epriestley added inline comments.
src/infrastructure/daemon/workers/PhabricatorTriggerDaemon.php
264

Maybe correct this to "sleep briefly" instead of "sleep for 60 seconds", since the actual value is currently 5 rather than 60.

(The 5 is probably debug code I neglected to strip out of D11872 since it appears in my test plan, but now that we do nuance imports here too I think it makes sense to leave it pretty small so nuance imports can be prompt.)

This revision is now accepted and ready to land.Apr 18 2016, 9:07 PM

Yes, that does look bad.
setOrder(PhabricatorWorkerTriggerQuery::ORDER_EXECUTION) doesn't imply "has NextEvent".
withNextEventBetween(0, null) means "nextEvent >=0", which is false for NULL in sql.

Maybe better to have an explicit ->withNextEvent($true) call instead of this funnyness?

This revision was automatically updated to reflect the committed changes.