Skip to content

Added PFFileStagingController to manage staging. #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 20, 2015
Merged

Added PFFileStagingController to manage staging. #49

merged 1 commit into from
Aug 20, 2015

Conversation

richardjrossiii
Copy link
Contributor

Previously, our file staging code was not centralized, wasn't very efficient, and most importantly made testing & migration difficult.

Fixes half of #18.

@nlutsenko
Copy link
Contributor

Since the delete is asynchronous - there is a potential race condition between copying/writing file to staging and nuking the directory - which could cause files to be lost.

What do you think if we would decouple everything related to staging files into PFFileStagingController, that would let us use a single synchronized access to file staging in general, as well as synchronize file deletion from that directory and creation of new files. It could live as a property on PFFileController, since it's not needed without that one.

@richardjrossiii richardjrossiii force-pushed the richardross.filestaging.clear branch from 1ba3d65 to fcdeecd Compare August 19, 2015 22:08
@richardjrossiii richardjrossiii changed the title Made PFCoreManager automatically clear its staging directory (asyncronously) upon initialization of Parse. Added PFFileStagingController to manage staging. Aug 19, 2015
@nlutsenko
Copy link
Contributor

Fix tests and pass back.

nlutsenko
nlutsenko reviewed Aug 19, 2015
View reviewed changes
Parse/Internal/File/Controller/PFFileStagingController.h
- (instancetype)init NS_UNAVAILABLE;
- (instancetype)initWithDataSource:(id<PFFileManagerProvider>)dataSource NS_DESIGNATED_INITIALIZER;

+ (instancetype)stagingControllerWithDataSource:(id<PFFileManagerProvider>)dataSource;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe use controllerWithDataSource: to be consistent with other controllers?

@richardjrossiii richardjrossiii force-pushed the richardross.filestaging.clear branch from fcdeecd to 27ec4d5 Compare August 19, 2015 23:34
nlutsenko
nlutsenko reviewed Aug 20, 2015
View reviewed changes
Tests/Unit/FileControllerTests.m
}

- (void)tearDown {
[[NSFileManager defaultManager] removeItemAtPath:[self temporaryDirectory] error:NULL];
[[PFFileManager removeItemAtPathAsync:[self temporaryDirectory]] waitUntilFinished];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was causing multiprocess file lock controller deadlocks :)

@nlutsenko
Copy link
Contributor

Few nits and an important piece about stagedFilePath property.

@richardjrossiii richardjrossiii force-pushed the richardross.filestaging.clear branch 4 times, most recently from e10791b to d9e170e Compare August 20, 2015 03:07
@richardjrossiii
Copy link
Contributor Author

This should fix tests.

nlutsenko
nlutsenko reviewed Aug 20, 2015
View reviewed changes
Parse/Internal/File/Controller/PFFileController.m
}

@end

@implementation PFFileController

@synthesize fileStagingController=_fileStagingController;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supernit: Add spaces around = to be inline with style in other places.

@richardjrossiii
Added PFFileStagingController to manage staging.
65b870e 
Previously, our file staging code was not centralized, wasn't very efficient, and most importantly made testing & migration difficult.

Fixes half of #18.
@richardjrossiii richardjrossiii force-pushed the richardross.filestaging.clear branch from d9e170e to 65b870e Compare August 20, 2015 03:12
@nlutsenko
Copy link
Contributor

Looks great, please ship.

richardjrossiii added a commit that referenced this pull request Aug 20, 2015
@richardjrossiii
Merge pull request #49 from ParsePlatform/richardross.filestaging.clear
f5d1279 
Added PFFileStagingController to manage staging.
@richardjrossiii richardjrossiii merged commit f5d1279 into master Aug 20, 2015
@richardjrossiii richardjrossiii deleted the richardross.filestaging.clear branch August 20, 2015 18:04
@nlutsenko nlutsenko added this to the 1.8.2 milestone Aug 26, 2015
@rdbayer rdbayer mentioned this pull request Sep 21, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@richardjrossiii richardjrossiii

Labels
None yet
Projects
None yet
Milestone
1.8.2
Development

Successfully merging this pull request may close these issues.
3 participants
@richardjrossiii @nlutsenko @facebook-github-bot