Page MenuHomePhabricator

When file transforms race and lose, accept defeat gracefully
ClosedPublic

Authored by epriestley on May 21 2015, 2:50 PM.

Details

Summary

Fixes T8277. Transforming files can race; resolve the race after we lose.

Test Plan
  • Added sleep(10) near the bottom of the transform controller.
  • Transformed a file in two browser windows at the same time; got something like this (exception corresponds to the loser of the race):

  • Applied patch.
  • Repeated process, got this:

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 31234.May 21 2015, 2:50 PM
epriestley retitled this revision from to When file transforms race and lose, accept defeat gracefully.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan accepted this revision.May 21 2015, 4:30 PM
btrahan edited edge metadata.
btrahan added inline comments.
src/applications/files/controller/PhabricatorFileTransformController.php
90

Is this more like a 500?

This revision is now accepted and ready to land.May 21 2015, 4:30 PM
epriestley added inline comments.May 21 2015, 4:42 PM
src/applications/files/controller/PhabricatorFileTransformController.php
90

My thinking on 404 is that we "should" only get here if the thing was just deleted, and we'd 404 if we were a fraction of a second quicker (on line 105, or after redirecting on line 111), and 404'ing in those cases makes fairly good sense to me.

In theory, we don't need to 404 here and could just loop back to the top and rebuild the transform, or loop here trying to either save our own transform or load a conflicting one, but those are both complicated and introduce the possibility that we might loop forever if there's a bug somewhere.

This revision was automatically updated to reflect the committed changes.