Page MenuHomePhabricator

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

Authored by epriestley on Sep 4 2021, 8:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 5:57 PM
Unknown Object (File)
Sun, Nov 24, 5:39 AM
Unknown Object (File)
Thu, Nov 21, 1:24 PM
Unknown Object (File)
Thu, Nov 21, 1:24 PM
Unknown Object (File)
Thu, Nov 21, 1:12 PM
Unknown Object (File)
Thu, Nov 21, 12:52 PM
Unknown Object (File)
Sat, Nov 16, 9:27 PM
Unknown Object (File)
Wed, Nov 6, 12:28 AM
Subscribers
None

Details

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.

Diff Detail

Repository
rARC Arcanist
Branch
future1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 25543
Build 35323: Run Core Tests
Build 35322: arc lint + arc unit

Event Timeline

I looked at Future to see how isReady() plays into other parts and this looks good! Thank you!

This revision is now accepted and ready to land.Sep 4 2021, 8:59 PM

Yeah -- isReady() should really be called something else nowadays (it's, uh, "doTheThingThisFutureDoes()" pretty much), but it would be a backward-compatibility break and I shuffled enough stuff around in T11968 that I didn't want to press my luck.

The return value of isReady() is still used today, but modern Futures are effectively ready to resolve once they have: called setResult(); or thrown an uncaught exception from isReady().

If isReady() throws an exception, the Future saves the exception, becomes ready to resolve (so FutureIterator will emit the future), stops updating, and re-throws the exception when resolved.