Page MenuHomePhabricator

When file transforms race and lose, accept defeat gracefully
ClosedPublic

Authored by epriestley on May 21 2015, 2:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 15, 12:42 PM
Unknown Object (File)
Feb 12 2024, 1:36 PM
Unknown Object (File)
Feb 5 2024, 8:42 AM
Unknown Object (File)
Feb 3 2024, 4:07 AM
Unknown Object (File)
Jan 10 2024, 1:23 PM
Unknown Object (File)
Jan 5 2024, 3:42 PM
Unknown Object (File)
Dec 26 2023, 9:58 AM
Unknown Object (File)
Dec 18 2023, 2:08 AM
Subscribers

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):

Screen Shot 2015-05-21 at 7.43.44 AM.png (947×1 px, 271 KB)

  • Applied patch.
  • Repeated process, got this:

Screen Shot 2015-05-21 at 7.48.00 AM.png (947×1 px, 77 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 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
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.