Differential D16271

Simplify "builtin file" management and recover from races
ClosedPublic
Actions

Authored by epriestley on Jul 11 2016, 4:19 PM.

Tags

None

Referenced Files

	F13083440: D16271.diff
	Wed, Apr 24, 10:32 PM

	Unknown Object (File)
	Sun, Apr 21, 4:28 PM

	Unknown Object (File)
	Wed, Apr 17, 4:36 PM

	Unknown Object (File)
	Wed, Apr 17, 2:09 PM

	Unknown Object (File)
	Fri, Apr 12, 6:36 AM

	Unknown Object (File)
	Sun, Apr 7, 5:19 PM

	Unknown Object (File)
	Mon, Apr 1, 4:50 PM

	Unknown Object (File)
	Mon, Apr 1, 3:19 AM

Subscribers

None

Details

Reviewers

Maniphest Tasks

T8124: Construction of "default" file resources may lose a race with itself
T11307: High-latency file storage can potentially break user profile pictures horribly

Commits

rP8ad61d01502d: Simplify "builtin file" management and recover from races

Summary

Fixes T11307. Fixes T8124. Currently, builtin files are tracked by using a special transform with an invalid source ID.

Just use a dedicated column instead. The transform thing is too clever/weird/hacky and exposes us to issues with the "file" and "transform" tables getting out of sync (possibly the issue in T11307?) and with race conditions.

Test Plan

Loaded profile "edit picture" page, saw builtins.
Deleted all builtin files, put 3 second sleep in the storage engine write, loaded profile page in two windows.
- Before patch: one of them failed with a race.
- After patch: both of them loaded.

Diff Detail

Repository

Branch

file2

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 13017
Build 16633: Run Core Tests
Build 16632: arc lint + arc unit

Event Timeline

epriestley updated this revision to Diff 39138.Jul 11 2016, 4:19 PM

epriestley retitled this revision from to Simplify "builtin file" management and recover from races.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added tasks: T11307: High-latency file storage can potentially break user profile pictures horribly, T8124: Construction of "default" file resources may lose a race with itself.

epriestley mentioned this in T11307: High-latency file storage can potentially break user profile pictures horribly.Jul 11 2016, 4:23 PM

chad accepted this revision.Jul 11 2016, 4:24 PM

chad edited edge metadata.

This revision is now accepted and ready to land.Jul 11 2016, 4:24 PM

Closed by commit rP8ad61d01502d: Simplify "builtin file" management and recover from races (authored by epriestley, committed by epriestley). · Explain WhyJul 11 2016, 4:25 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

resources/

sql/

autopatches/

20160711.files.01.builtin.sql

2 lines

20160711.files.02.builtinkey.sql

2 lines

src/

applications/

files/

query/

PhabricatorFileQuery.php

13 lines

storage/

PhabricatorFile.php

38 lines

Diff 39138

resources/sql/autopatches/20160711.files.01.builtin.sql

Loading...

resources/sql/autopatches/20160711.files.02.builtinkey.sql

Loading...

src/applications/files/query/PhabricatorFileQuery.php

Loading...

src/applications/files/storage/PhabricatorFile.php

Loading...