Fix some daemon errors related to multiple/out-of-order events
ClosedPublic
Actions

Authored by epriestley on May 26 2016, 1:13 AM.

Details

Reviewers

chad

Maniphest Tasks

T10811: `phd-daemon` crash after automatic restart following config change

Commits

rPHU05e30630a4d3: Fix some daemon errors related to multiple/out-of-order events

Summary

Ref T10811. Currently, we can emit the "willExit" event multiple times, or emit it and then emit a "log" event.

This causes some problems with the Phabricator listener, which cleans up resources the first time it sees this event. When it gets the second copy (or logs afterward) it fails to process events.

Instead, emit it only once we're actually cleaning up the daemon handle. This guarantees it emits last and emits exactly once.

I believe this only cleans up logs, but it's possible it also improves stability.

Test Plan

You can reproduce this issue like this:

Run bin/phd debug hangforever.
Kill it with ^C^C.
Most of the time, it will emit errors.

With --trace, you can see additional information. After this patch, it no longer emits errors but still successfully marks the daemon as exited, and the willExit event is always emitted exactly once and emitted last.

Diff Detail

Repository

rPHU libphutil

Branch

exit1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 12335
Build 15601: Run Core Tests
Build 15600: arc lint + arc unit

Event Timeline

epriestley updated this revision to Diff 38460.May 26 2016, 1:13 AM

epriestley retitled this revision from to Fix some daemon errors related to multiple/out-of-order events.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T10811: `phd-daemon` crash after automatic restart following config change.

Specifically, the error this fixes looks like this:

[2016-05-25 17:45:35] EXCEPTION: (Exception) No such daemon "39084:6ra7uz"! at [<phabricator>/src/applications/daemon/event/PhabricatorDaemonEventListener.php:120]
arcanist(head=master, ref.master=2234c8cacc21), corgi(head=master, ref.master=cfed644883b0), instances(head=stable, ref.master=47349cf90a41, ref.stable=788b3aa42b2a), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=stable, ref.master=189600e4116e, ref.stable=9f399247b1b1, custom=8), phutil(head=locale1, ref.master=bd56873ae4c0, ref.locale1=092a19c216e9), services(head=master, ref.master=e07ec904b0b9)
  #0 <#2> PhabricatorDaemonEventListener::getDaemon(string) called at [<phabricator>/src/applications/daemon/event/PhabricatorDaemonEventListener.php:63]
  #1 <#2> PhabricatorDaemonEventListener::handleLogEvent(PhutilEvent) called at [<phabricator>/src/applications/daemon/event/PhabricatorDaemonEventListener.php:24]
  #2 <#2> PhabricatorDaemonEventListener::handleEvent(PhutilEvent) called at [<phutil>/src/events/PhutilEventEngine.php:65]
  #3 <#2> PhutilEventEngine::dispatchEvent(PhutilEvent) called at [<phutil>/src/daemon/PhutilDaemonHandle.php:248]
  #4 phlog(Exception) called at [<phutil>/src/daemon/PhutilDaemonHandle.php:250]
  #5 PhutilDaemonHandle::dispatchEvent(string, array) called at [<phutil>/src/daemon/PhutilDaemonHandle.php:418]
  #6 PhutilDaemonHandle::logMessage(string, string) called at [<phutil>/src/daemon/PhutilDaemonHandle.php:117]
  #7 PhutilDaemonHandle::update() called at [<phutil>/src/daemon/PhutilDaemonOverseer.php:209]
  #8 PhutilDaemonOverseer::run() called at [<phabricator>/scripts/daemon/launch_daemon.php:18]

That happened because we'd emit the event once, and throw away 39084:6ra7uz while processing it. Then we'd emit the event again (or emit a log event) and the listener would try to look up 39084:6ra7uz, but fail because we just threw it away.

chad accepted this revision.May 26 2016, 3:01 PM

chad edited edge metadata.

This revision is now accepted and ready to land.May 26 2016, 3:01 PM

Closed by commit rPHU05e30630a4d3: Fix some daemon errors related to multiple/out-of-order events (authored by epriestley, committed by epriestley). · Explain WhyMay 26 2016, 3:08 PM

This revision was automatically updated to reflect the committed changes.