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
F14091353: D7688.diff
Sun, Nov 24, 11:31 PM
Unknown Object (File)
Fri, Nov 22, 11:31 AM
Unknown Object (File)
Oct 18 2024, 9:58 PM
Unknown Object (File)
Oct 17 2024, 8:53 PM
Unknown Object (File)
Oct 10 2024, 8:06 AM
Unknown Object (File)
Oct 9 2024, 3:03 PM
Unknown Object (File)
Oct 5 2024, 1:50 AM
Unknown Object (File)
Oct 3 2024, 6:08 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

Lint
Lint Skipped
Unit
Tests Skipped

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