Page MenuHomePhabricator

Use "MethodCallFuture" to move Diffusion Conduit exceptions to resolution time
ClosedPublic

Authored by epriestley on Sep 4 2021, 9:13 PM.
Tags
None
Referenced Files
F14091625: D21721.diff
Mon, Nov 25, 1:08 AM
Unknown Object (File)
Wed, Nov 6, 12:28 AM
Unknown Object (File)
Wed, Nov 6, 12:28 AM
Unknown Object (File)
Wed, Nov 6, 12:28 AM
Unknown Object (File)
Wed, Nov 6, 12:28 AM
Unknown Object (File)
Mon, Nov 4, 10:51 AM
Unknown Object (File)
Thu, Oct 31, 2:09 AM
Unknown Object (File)
Oct 24 2024, 7:02 AM
Subscribers
None

Details

Summary

Depends on D21720. Ref T13666. See D21720 for additional discussion.

Use "MethodCallFuture", introduced in D21720, so that exceptions raised in "execute()" are thrown when the future is resolved, not when the future is created.

This makes exception behavior for clustered and non-clustered setups consistent, and chooses the intended (clustered) behavior in both cases, which currently deals with errors better.

Test Plan
  • Applied both parts of the patch in T13666 (break history queries, force immediate futures) to reproduce the issue in T13666.
    • Loaded a Diffusion landing page, reproduced the error described in that task.
  • Applied this patch, verified landing page works again.
  • Removed the "break history queries" change, verified landing page works in forced-immediate mode.
  • Removed the "force immediate" change, verified landing page works in "actual future" mode.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 4 2021, 9:14 PM
Harbormaster failed remote builds in B25544: Diff 51746!

There's a possible argument here that we shouldn't have this branch at all. Even in non-clustered mode, we can (in theory) use a real future to make a call to ourselves -- and (in theory) this may be faster than executing the call inline (this branch is effectively executing all calls in a single-threaded serial way in-process).

I think it works this way because:

  • when this stuff was first written, it was helpful to isolate the newer clustering code from the existing tried-and-true method;
  • we can't really guarantee non-clustered setups can actually make HTTP calls to themselves, and we don't need this capability, and it's kind of a mess to debug if it's broken somewhere, I guess?; and
  • in practice, "real conduit" has a lot of overhead today and is probably not faster for installs small enough to not need clustering.

None of these are amazing arguments for keeping this (and I think "fewer execution paths is better than more execution paths" is a pretty good argument for getting rid of it) but there's enough general messiness here that I feel like this less-ambitious approach to just fixing the future semantics is well within the realm of reasonable changes.

  • (Re-run remote tests now that "MethodCallFuture" exists upstream.)

Thanks

I hadn't considered that having this still use http requests could be done in parallel vs. the clustered running them in serial being faster (theoretically)

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