Page MenuHomePhabricator

Prevent caching of workflows
ClosedPublic

Authored by BYK on Sep 1 2015, 8:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Aug 16, 4:22 AM
Unknown Object (File)
Thu, Aug 4, 3:28 PM
Unknown Object (File)
Wed, Aug 3, 5:22 AM
Unknown Object (File)
Sun, Jul 31, 4:24 PM
Unknown Object (File)
Thu, Jul 21, 4:59 PM
Unknown Object (File)
Jul 17 2022, 2:32 PM
Unknown Object (File)
Jul 14 2022, 3:55 AM
Unknown Object (File)
Jul 13 2022, 10:43 AM
Tokens
"Like" token, awarded by joshuaspence.

Details

Summary

Fixes T9159.

Test Plan

Run arc patch on a code base requiring auth for a diff that has at least one dependency.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

BYK retitled this revision from to Only set `$workflow-conduit` if it's missing.
BYK updated this object.
BYK edited the test plan for this revision. (Show Details)
BYK added a reviewer: epriestley.
BYK edited edge metadata.
  • don't skip lint and unit

Under what scenario is $workflow->conduit not null here?

@avivey - When arc patch runs itself recursively. The thing I realized is arc patch first tries to get the patch without using credentials (but it still requires a conduit), and then when it realizes the request is rejected due to missing credentials, it manually adds them by calling authenticateConduit. After that it calls run, which in turn calls buildChildWorkflow. Since the parent buildChildWorkflow doesn't know about the already established conduit which has its credentials already set ($workflow->conduit being set) it simply tries to reset it causing $workflow->setConduitCredentials($this->conduitCredentials); call to throw the You may not set new credentials after authenticating conduit. error since credentials were already set manually by arc patch.

Does this make sense? I may be wrong about this trace but it looks very much like the case.

Ok, this is a little heavy on context. tl;dr: This is a symptom.

We have a leaky abstraction, where buildWorkflow() is cached, returning an already-built and used workflow instance. Overwriting the conduit is a symptom of this cache.

Similar, and worse, behaviors, will turn up in more complicated flows, where the same Workflow class is expected to be run in multiple configurations (possibly in parallel). One hypothetical example could be a cross-repository-diff-workflow; arc patch-with-dependencies is obviously another one.

A possible solution might be to have buildWorkflow() return a new instance, which will seal this leak, but that might have more side-effects to consider.

epriestley edited edge metadata.

Building a separate Conduit client also feels weird to me -- it feels surprising that workflows are cached (which feels pointless and fragile) but the Conduit client is not (which feels expected and desirable).

I suspect these decisions are cruft or poor judgement from ages long past, not meaningful architectural choices.

In particular, it seems very likely that the workflow caching is a consequence of moving ArcanistConfiguration to use PhutilClassMapQuery (which caches, while PhutilSymbolLoader did not), and that the one-line fix is probably to add clone to ArcanistConfiguration::buildWorkflow(). The structure of this class generally predates modern/common use of modular classes and doesn't do a good job of separating the concepts of "load the available implementations for read-only operations" vs "give me an instance of something so I can write to it". This is something we do a somewhat weak job of in general, but probably ~90% of the extension points are always read-only and I think we've always caught the need to clone the other 10%, so far. This might merit some thought and formalization of conventions if it occurs a couple more times, though.

In this case, I'd say the best quick fix is to clone in either buildWorkflow() or buildAllWorkflows(), to restore the pre-PhutilClassMapQuery semantics. It appears that all callers of buildAllWorkflows() use it in read-only way, so putting it in buildWorkflow() is likely sufficient:

diff --git a/src/configuration/ArcanistConfiguration.php b/src/configuration/ArcanistConfiguration.php
index 91d0dc6..e66b793 100644
--- a/src/configuration/ArcanistConfiguration.php
+++ b/src/configuration/ArcanistConfiguration.php
@@ -31,7 +31,7 @@ class ArcanistConfiguration extends Phobject {
       $command = 'version';
     }
 
-    return idx($this->buildAllWorkflows(), $command);
+    return clone idx($this->buildAllWorkflows(), $command);
   }
 
   public function buildAllWorkflows() {

Can you confirm that fixes the arc patch issue?

Beyond the immediate issue, we should probably eventually:

  • share a single Conduit client across workflows, at least by default;
  • eventually realign arc extension points with other modern APIs, but this will break some users -- this likely makes sense concurrent with T5055;
  • keep an eye on further clone issues and consider technical or conventional approaches to reducing their frequency and impact if they continue occuring.
This revision now requires changes to proceed.Sep 11 2015, 4:24 AM

Basically, just what @avivey said, with one additional note: the move to PhutilClassMapQuery in rARC0e095787 (less than a month ago) is almost certainly the root cause of the caching, so reverting this is almost certainly straightforward and not risking years of weird side effects coming home to roost.

@epriestley - so are you saying I should simply revert rARC0e0957872083daa38309204ad9774568c9fcdd5c? I mean that looks like a nice patch and given all the context you and @avivey shared (thanks a ton btw for that), I may be able to prevent caching "properly".

Which direction would you prefer?

Oh, just:

  • Test the one-line clone diff above.
  • If it works, replace this one-line change with that one-line change and I'll upstream it.
BYK edited edge metadata.
  • Do not cache workflow
BYK retitled this revision from Only set `$workflow-conduit` if it's missing to Prevent caching of workflows.Sep 15 2015, 12:01 PM
BYK edited edge metadata.
This revision is now accepted and ready to land.Sep 15 2015, 12:23 PM
This revision was automatically updated to reflect the committed changes.

Thank you guys for the pointer and quick turn around!