Skip to content

[flutter_tools] implement safe file copy with multiple fallbacks#69000

Merged
jonahwilliams merged 4 commits intoflutter:masterfrom
jonahwilliams:safe_copy_123
Oct 26, 2020
Merged

[flutter_tools] implement safe file copy with multiple fallbacks#69000
jonahwilliams merged 4 commits intoflutter:masterfrom
jonahwilliams:safe_copy_123

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Oct 25, 2020

Description

The tool observes a large number of unhandled exceptions during the file copy portion of flutter create. it is difficult to tell whether the permission issue is caused by the source/destination, or whether it is due to a bug in dart:io.

To work around this, implement a permission check for both the source and dest files. If either fails, the tool can exit with a more specific message.

If these checks pass, then perform the actual copy. If the copy fails, fallback to manually copying the bytes

@flutter-dashboard flutter-dashboard bot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Oct 25, 2020
@google-cla google-cla bot added the cla: yes label Oct 25, 2020
@jonahwilliams jonahwilliams marked this pull request as ready for review October 26, 2020 15:53
try {
return wrapFile(delegate.copySync(newPath));
} on FileSystemException {
// Proceed below
Copy link
Member

Choose a reason for hiding this comment

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

If we had more information about this case, we could report a good bug. Consider recording an analytics event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jonahwilliams
Copy link
Contributor Author

If we do not see the fallback event in analytics after a stable cycle or so, we can delete the fallback code

// Proceed below
}
// If the copy failed but both of the above checks passed, copy the bytes
// directly.
Copy link
Member

Choose a reason for hiding this comment

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

If something goes wrong with this, we should probably delete the destination file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}, platform: _platform, failureMessage: 'Flutter failed to copy $path to $newPath due to unknown error');
// The original copy failed, but the manual copy worked. Report an analytics event to
// track this to determine if this code path is actually hit.
ErrorHandlingEvent('copy-fallback').send();
Copy link
Member

Choose a reason for hiding this comment

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

This will give us the count of copySync failing, but the manual copy succeeding. Would any other information be helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If one of the file open checks fails, it is likely to be a regular old permission issue. If not, crash logging will handle it.

if the fallback copySync fails, assuming it is not one of the known issues, crash logging will handle that too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants