Page MenuHomePhabricator

Streamline handling of Futures and PIDs in daemons
ClosedPublic

Authored by epriestley on Jul 23 2020, 5:40 PM.
Tags
None
Referenced Files
F12802544: D21425.diff
Wed, Mar 27, 12:39 PM
Unknown Object (File)
Feb 20 2024, 12:46 PM
Unknown Object (File)
Feb 8 2024, 6:44 AM
Unknown Object (File)
Feb 4 2024, 12:25 AM
Unknown Object (File)
Jan 31 2024, 5:31 AM
Unknown Object (File)
Jan 22 2024, 5:07 AM
Unknown Object (File)
Jan 16 2024, 7:17 PM
Unknown Object (File)
Jan 5 2024, 1:19 PM
Subscribers

Details

Summary

Ref T13555. Currently, the daemon future may resolve into a failure state immediately inside "start()", and not have a valid PID when we read it.

Instead, read PIDs from the current active future in all cases, using "hasPID()" to test for the presence of a valid PID.

Since we don't query the PID immediately, we no longer need to explicitly start the future.

Also fix an issue where the same future could be added to the overseer pool more than once if it threw on "resolve()". In general:

  • Before we "resolve()" a future, detach it from the DaemonHandle: we're always done with it.
  • Catch exceptions on resolution and treat them the same way as subprocess resolution errors. These aren't common, but are possible in the general case.
  • Have DaemonHandle add futures to the future pool directly when they're created.
Test Plan
  • Ran daemons with intentional subprocess creation failures, saw clean recovery.
  • Ran daemons with intentional resolution exceptions, saw clean recovery.

Diff Detail

Repository
rP Phabricator
Branch
pid2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 24797
Build 34210: Run Core Tests
Build 34209: arc lint + arc unit

Event Timeline

  • Use "$caught", not "$ex", to make the code less fragile.
This revision was not accepted when it landed; it landed in state Needs Review.Jul 23 2020, 6:22 PM
This revision was automatically updated to reflect the committed changes.
jmeador added inline comments.
src/infrastructure/daemon/PhutilDaemonHandle.php
154

canResolve doesn't seem to exist anywhere in the codebase... but my daemons work fine. What gives?

The Future stuff is used on both the client and server, so it lives in Arcanist rather than Phabricator. The method definition should be here:

https://secure.phabricator.com/diffusion/ARC/browse/master/src/future/Future.php$253