Page MenuHomePhabricator

Ensure files live as long as intended.
Needs RevisionPublic

Authored by isfs on Jul 6 2016, 11:06 PM.

Details

Summary

Previously, if a temporary file was created, but the same data was later intended to be permanent (e.g. a temporary project icon created for the Edit Picture page was assigned to a project for continued use), the existing temporary file would be retrieved by content hash, and then later cleaned up by the garbage collector undesirably. This change adjusts the file expiry time when retrieving exising files by content hash so they live as long as if the file did not exist but were newly created.

Fixes T10907.

Test Plan
  • Without the patch applied:
    • Assign a default picture to a project which is missing one
    • Navigate to the file and note that it is temporary (if it isn't, change the colour+icon combination of the project to one which hasn't been used before and repeat from the start)
  • Apply the patch
  • Assign the default picture to the project again
  • Navigate to the file again and note that it is no longer temporary

Plus added a unit test. It is a little ugly due to how data types seem to work in LiskDAO.

Diff Detail

Repository
rP Phabricator
Branch
temporary-file-fix
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 12948
Build 16527: arc lint + arc unit

Event Timeline

isfs retitled this revision from to Ensure files live as long as intended..
isfs updated this object.
isfs edited the test plan for this revision. (Show Details)
isfs added a reviewer: epriestley.
cspeckmim added inline comments.
src/applications/files/storage/PhabricatorFile.php
234

I don't quite follow - we only change the TTL if it previously had a TTL?

src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
419

Is this intended to be the same value as line 392?

src/applications/files/storage/PhabricatorFile.php
234

Yes. If it doesn't have a TTL it is a permanent file so will last 'forever'. If something else is relying on the file persisting, we don't want to shorten its life by giving it a TTL.

src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
419

Any value less than that at 409 would do; I just picked the same value as at 392 for ease.

Thanks for the clarifications

src/applications/files/storage/PhabricatorFile.php
234

Aha, it clicked for me - for some reason it wasn't when initially reading through all your descriptions.

src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
419

Ah I think I originally misread this as comparing new TTL to the old, which this is explicitly confirming the value isn't changed.

epriestley edited edge metadata.

This analysis is correct, but I think D16270 is a better fix.

I'll accept separate patches to:

  • fix the documentation mistake;
  • fix the variable typo;
  • rename buildFromFileDataOrHash(...) to something more clear, like newTemporaryFileFromData(...).
This revision now requires changes to proceed.Jul 11 2016, 3:26 PM
  • fix the variable typo;

This problem has gone away due to other recent work.

  • fix the documentation mistake;

Can do.

  • rename buildFromFileDataOrHash(...) to something more clear, like newTemporaryFileFromData(...).

IMHO, the race condition should be fixed. Though I suppose it will be very rare, if the TTL/expiry isn't adjusted when a temporary file is 'retrieved' rather than 'created', 7 days (default) after creation, if the file is retrieved an about-to-expire file will be returned; in fact, an already-expired file could even be returned if the garbage collector hasn't run yet. If the GC then manages to run between the file generation and the browser requesting the referenced file, of course it will break. It's a less severe breakage, at least in the icon case, because it only breaks the icon selection page, but the crystal ball is foggy regarding future applications. I guess in any conceivable case the user would just refresh and it would work. But intermittent failures are horrid, make the software seem flaky/unreliable, and are hard to track down. IMHO, better to fix the race condition now that we've found it. If client code expects a file to stick around for a certain time, it should do so, not pull the carpet out from under it.

I could amend this patch to:

  • remove buildFromFileDataOrHash entirely and always create new files; they would still share storage, of course, but it could churn through file IDs faster than desirable, perhaps; OR
  • extend the expiry only, never to make files permanent (and rename the method); OR
  • do some kind of hysteresis thing, where the expiry is only extended if it falls short by a significant margin, but I think that's overkill.

Please let me know if you're amenable to any of those ideas.