recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Recover from a race when importing external objects (like JIRA issues) for the first time

Summary:
Fixes T11604. If we send two requests to render a brand new tag at about the same time (say, 50ms apart) but JIRA takes more than 50ms to return from its API call, the two processes will race one another and try to save the same external object.

If they do, have whichever one lost the race just load the object the other one created.

Apply this to other bridges, too.

Test Plan:
- Created a new task in JIRA.
- Referenced it for the first time in Differential, in a comment.
- This causes two tag renders to fire. This //might// be a bug but I spend 30 seconds on it without figuring out what was up. Regardless, we should fix the race even if the reason it's triggering so easily legitimately is a bug.
- Before patch: big error dialog (as in T11604).
- After patch: smooth sailing.

{F1804008}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11604

Differential Revision: https://secure.phabricator.com/D16514

epriestley 138efb2b e2430d0a

+32 -16
+28
src/applications/doorkeeper/bridge/DoorkeeperBridge.php
··· 48 48 return null; 49 49 } 50 50 51 + final protected function saveExternalObject( 52 + DoorkeeperObjectRef $ref, 53 + DoorkeeperExternalObject $obj) { 54 + 55 + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); 56 + try { 57 + $obj->save(); 58 + } catch (AphrontDuplicateKeyQueryException $ex) { 59 + 60 + // In various cases, we may race another process importing the same 61 + // data. If we do, we'll collide on the object key. Load the object 62 + // the other process created and use that. 63 + $obj = id(new DoorkeeperExternalObjectQuery()) 64 + ->setViewer($this->getViewer()) 65 + ->withObjectKeys(array($ref->getObjectKey())) 66 + ->executeOne(); 67 + if (!$obj) { 68 + throw new PhutilProxyException( 69 + pht('Failed to load external object after collision.'), 70 + $ex); 71 + } 72 + 73 + $ref->attachExternalObject($obj); 74 + } 75 + unset($unguarded); 76 + } 77 + 78 + 51 79 }
+1 -4
src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
··· 113 113 } 114 114 115 115 $this->fillObjectFromData($obj, $result); 116 - 117 - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); 118 - $obj->save(); 119 - unset($unguarded); 116 + $this->saveExternalObject($ref, $obj); 120 117 } 121 118 } 122 119
+1 -4
src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php
··· 78 78 $obj = $ref->getExternalObject(); 79 79 80 80 $this->fillObjectFromData($obj, $result); 81 - 82 - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); 83 - $obj->save(); 84 - unset($unguarded); 81 + $this->saveExternalObject($ref, $obj); 85 82 } 86 83 } 87 84
+1 -4
src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php
··· 101 101 102 102 $obj = $ref->getExternalObject(); 103 103 $this->fillObjectFromData($obj, $spec); 104 - 105 - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); 106 - $obj->save(); 107 - unset($unguarded); 104 + $this->saveExternalObject($ref, $obj); 108 105 } 109 106 } 110 107
+1 -4
src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
··· 104 104 } 105 105 106 106 $this->fillObjectFromData($obj, $result); 107 - 108 - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); 109 - $obj->save(); 110 - unset($unguarded); 107 + $this->saveExternalObject($ref, $obj); 111 108 } 112 109 } 113 110