Page MenuHomePhabricator

Make it safe to call ExecFuture->resolveKill() after process resolution
ClosedPublic

Authored by epriestley on Dec 3 2013, 12:39 PM.
Tags
None
Referenced Files
F17540691: D7688.id17369.diff
Thu, Jul 3, 12:54 PM
Unknown Object (File)
May 8 2025, 7:33 AM
Unknown Object (File)
Apr 27 2025, 11:27 PM
Unknown Object (File)
Apr 24 2025, 9:20 AM
Unknown Object (File)
Apr 11 2025, 6:59 PM
Unknown Object (File)
Apr 8 2025, 3:15 PM
Unknown Object (File)
Apr 7 2025, 8:24 AM
Unknown Object (File)
Apr 6 2025, 3:40 PM
Subscribers

Details

Summary

Fixes T4165. Currently, doing something like this is fine:

$exec_future->resolve();
$exec_future->resolve();

...but this isn't:

$exec_future->resolve();
$exec_future->resolveKill();

...and will raise garbage in the resolveKill() call. This is unhelpful, unexpected, and undesirable. Calling resolve*() multiple times is explicitly allowed, since it makes many patterns much easier to write and isn't sketchy or surprising or anything.

Check for an existing result before performing the kill.

Test Plan

Added a failing unit test and made it pass.

Diff Detail

Branch
multiresolve
Lint
Lint Passed
Unit
Tests Passed

Event Timeline

btrahan added inline comments.
src/future/exec/ExecFuture.php
415

I figured the process would still get closed though, we just don't need to make the result above? (ie move the closing bracket on the new if to be above line 415) not sure if closeProcess does other stuff that is bad news.

src/future/exec/ExecFuture.php
415

If we have a result already, the process has been closed elsewhere. Everything which generates a result also closes the process.

src/future/exec/ExecFuture.php
415

gotchya