1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-21 22:32:41 +01:00

Provide "MethodCallFuture" to fix exception semantics in mixed-future contexts

Summary:
Ref T13666. Currently, "PhabricatorRepository->newConduitFuture()" sometimes returns a real "ConduitFuture" (when repository clustering is configured) and sometimes returns a degenerate "ImmediateFuture" (when repository clustering is not configured).

To populate the "ImmediateFuture", the Conduit method is called inline without any special exception handling. This means that the two pathways have significantly different exception behavior:

  - On the "ImmediateFuture" pathway, the "newConduitFuture()" method itself may raise an exception related to resolving the call (e.g., invalid parameters).
  - On the "ConduitFuture" pathway, exceptions are raised only once the future is "resolve()"'d.

The second behavior is the correct behavior (and the primary behavior of upstream test environments, since all my stuff and the Phacility stuff is clustered).

Prepare to put both pathways on the second behavior by introducing a "MethodCallFuture", which can move the `$conduit_call->execute()` call (and thus its exception handling) to future resolution time.

See also the followup diff in Phabricator to actually enact this change.

Test Plan: Added unit tests and made them pass, see also next change.

Reviewers: cspeckmim

Reviewed By: cspeckmim

Maniphest Tasks: T13666

Differential Revision: https://secure.phabricator.com/D21720
This commit is contained in:
epriestley 2021-09-04 13:22:43 -07:00
parent 82016c00e1
commit f993b1fbda
3 changed files with 97 additions and 0 deletions

View file

@ -632,6 +632,8 @@ phutil_register_library_map(array(
'LinesOfALargeFile' => 'filesystem/linesofalarge/LinesOfALargeFile.php',
'LinesOfALargeFileTestCase' => 'filesystem/linesofalarge/__tests__/LinesOfALargeFileTestCase.php',
'MFilterTestHelper' => 'utils/__tests__/MFilterTestHelper.php',
'MethodCallFuture' => 'future/MethodCallFuture.php',
'MethodCallFutureTestCase' => 'future/__tests__/MethodCallFutureTestCase.php',
'NoseTestEngine' => 'unit/engine/NoseTestEngine.php',
'PHPASTParserTestCase' => 'parser/xhpast/__tests__/PHPASTParserTestCase.php',
'PhageAction' => 'phage/action/PhageAction.php',
@ -1695,6 +1697,8 @@ phutil_register_library_map(array(
'LinesOfALargeFile' => 'LinesOfALarge',
'LinesOfALargeFileTestCase' => 'PhutilTestCase',
'MFilterTestHelper' => 'Phobject',
'MethodCallFuture' => 'Future',
'MethodCallFutureTestCase' => 'PhutilTestCase',
'NoseTestEngine' => 'ArcanistUnitTestEngine',
'PHPASTParserTestCase' => 'PhutilTestCase',
'PhageAction' => 'Phobject',

View file

@ -0,0 +1,36 @@
<?php
/**
* Degenerate future which resolves by calling a method.
*
* $future = new MethodCallFuture($calculator, 'add', 1, 2);
*
* This future is similar to @{class:ImmediateFuture}, but may make it easier
* to implement exception behavior correctly. See T13666.
*/
final class MethodCallFuture extends Future {
private $callObject;
private $callMethod;
private $callArgv;
public function __construct($object, $method /* , ...*/ ) {
$argv = func_get_args();
$this->callObject = $object;
$this->callMethod = $method;
$this->callArgv = array_slice($argv, 2);
}
public function isReady() {
$call = array($this->callObject, $this->callMethod);
$argv = $this->callArgv;
$result = call_user_func_array($call, $argv);
$this->setResult($result);
return true;
}
}

View file

@ -0,0 +1,57 @@
<?php
final class MethodCallFutureTestCase extends PhutilTestCase {
public function testMethodCallFutureSums() {
$future = new MethodCallFuture($this, 'getSum', 1, 2, 3);
$result = $future->resolve();
$this->assertEqual(6, $result, pht('MethodCallFuture: getSum(1, 2, 3)'));
$future = new MethodCallFuture($this, 'getSum');
$result = $future->resolve();
$this->assertEqual(0, $result, pht('MethodCallFuture: getSum()'));
}
public function testMethodCallFutureExceptions() {
$future = new MethodCallFuture($this, 'raiseException');
// See T13666. Using "FutureIterator" to advance the future until it is
// ready to resolve should NOT throw an exception.
foreach (new FutureIterator(array($future)) as $resolvable) {
// Continue below...
}
$caught = null;
try {
$future->resolve();
} catch (PhutilMethodNotImplementedException $ex) {
$caught = $ex;
}
$this->assertTrue(
($caught instanceof PhutilMethodNotImplementedException),
pht('MethodCallFuture: exceptions raise at resolution.'));
}
public function getSum(/* ... */) {
$args = func_get_args();
$sum = 0;
foreach ($args as $arg) {
$sum += $arg;
}
return $sum;
}
public function raiseException() {
// We just want to throw any narrow exception so the test isn't catching
// too broad an exception type. This is simulating some exception during
// future resolution.
throw new PhutilMethodNotImplementedException();
}
}