Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Implement WriteAtomically using write/fsync on all platforms, and enable file unittests on Fuchsia #18320

Merged
merged 1 commit into from
May 21, 2020
Merged

Implement WriteAtomically using write/fsync on all platforms, and enable file unittests on Fuchsia #18320

merged 1 commit into from
May 21, 2020

Conversation

@gw280
Copy link
Contributor

@gw280 gw280 commented May 12, 2020

This is because the existing mmap/memcpy codepath doesn't work on Fuchsia.

@gw280 gw280 requested a review from chinmaygarde May 12, 2020 22:38
@auto-assign auto-assign bot requested a review from jason-simmons May 12, 2020 22:38
chinmaygarde
chinmaygarde suggested changes May 12, 2020
View reviewed changes
fml/platform/posix/file_posix.cc Outdated
FileMapping mapping(temp_file, {FileMapping::Protection::kWrite});
if (mapping.GetMutableMapping() == nullptr ||
data.GetSize() != mapping.GetSize()) {
ssize_t size = ::write(temp_file.get(), data.GetMapping(), data.GetSize());
Copy link
Member

@chinmaygarde chinmaygarde May 12, 2020

Choose a reason for hiding this comment

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

size may be less than the data.GetSize(). if its not -1 and less than data.GetSize(), there must be a loop that writes the entire data contents. Otherwise, the file contents are truncated and corrupt. Refer to the man page for write for details.

Copy link
Contributor Author

@gw280 gw280 May 18, 2020

Choose a reason for hiding this comment

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

I added the loop but I'm not clear on if we can assume the file is seekable as it relies on the file offset being updated:

For a seekable file (i.e., one to which lseek(2) may be applied, for example, a regular file) writing takes place at the current file offset, and the file offset is incremented by the number of bytes actually written.

@gw280 gw280 force-pushed the gwright-writeatomically branch from a814ee6 to 3aea706 Compare May 18, 2020 22:01
@gw280 gw280 requested a review from chinmaygarde May 18, 2020 22:02
@gw280 gw280 force-pushed the gwright-writeatomically branch from 3aea706 to 18789d7 Compare May 20, 2020 18:07
chinmaygarde
chinmaygarde approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

LGTM other than the missing EINTR check.

fml/platform/posix/file_posix.cc Outdated
ssize_t offset = 0;

while (remaining > 0) {
written = ::write(temp_file.get(), data.GetMapping() + offset, remaining);
Copy link
Member

@chinmaygarde chinmaygarde May 20, 2020

Choose a reason for hiding this comment

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

Missing EINTR check.

@gw280 gw280 force-pushed the gwright-writeatomically branch from 24fb788 to f5f01d4 Compare May 21, 2020 17:33
@gw280 gw280 added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 21, 2020
@fluttergithubbot fluttergithubbot merged commit 80093aa into flutter:master May 21, 2020
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 21, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 21, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
e8cccf7 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
dd5c10b 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
3a32da8 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
948c127 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
d24de6a 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
62c5cd6 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 22, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
62c5b6d 
… and enable (flutter/engine#18320)
wandyers pushed a commit to wandyers/engine that referenced this pull request May 23, 2020
@wandyers
Implement WriteAtomically using write/fsync on all platforms, and ena…
9aac9c4 
…ble (flutter#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 23, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
7db3e5d 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 26, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
04674a9 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 26, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
79ab7f1 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 26, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
f2c1a0f 
… and enable (flutter/engine#18320)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 26, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2020
@engine-flutter-autoroll
80093aa Implement WriteAtomically using write/fsync on all platforms,…
7924557 
… and enable (flutter/engine#18320)
liyuqian added a commit to liyuqian/flutter that referenced this pull request May 26, 2020
@liyuqian
Roll Engine from 9ce1e5c to 1a83498 (69 revisions)
0516672 
Roll Engine from 9ce1e5c to 1a83498 (69 revisions)

flutter/engine@9ce1e5c...1a83498

2020-05-26 [email protected] Revert "Update CanvasKit to 0.15.0 (flutter#18570)" (flutter/engine#18600)
2020-05-26 [email protected] Roll Fuchsia Mac SDK from TSEOq... to hr-HZ... (flutter/engine#18594)
2020-05-26 [email protected] Roll Dart SDK from 65113fd73dec to 9e3b0289197d (2 revisions) (flutter/engine#18593)
2020-05-26 [email protected] Roll Skia from b6d158aaf3dd to e4b4ca1050b9 (1 revision) (flutter/engine#18592)
2020-05-26 [email protected] Roll Fuchsia Linux SDK from DzG49... to pLp67... (flutter/engine#18591)
2020-05-24 [email protected] Roll Fuchsia Mac SDK from Oz016... to TSEOq... (flutter/engine#18590)
2020-05-23 [email protected] Roll Skia from afd8a6c6ae87 to b6d158aaf3dd (3 revisions) (flutter/engine#18588)
2020-05-22 [email protected] Roll Fuchsia Mac SDK from 99Z4_... to Oz016... (flutter/engine#18582)
2020-05-22 [email protected] Roll Skia from ec31488ace66 to afd8a6c6ae87 (1 revision) (flutter/engine#18581)
2020-05-22 [email protected] Roll Fuchsia Linux SDK from Kvwlr... to DzG49... (flutter/engine#18580)
2020-05-22 [email protected] Roll Skia from 80abb89c3632 to ec31488ace66 (1 revision) (flutter/engine#18579)
2020-05-22 [email protected] Roll Skia from da90c3765908 to 80abb89c3632 (4 revisions) (flutter/engine#18577)
2020-05-22 [email protected] Roll Fuchsia Mac SDK from _zNmv... to 99Z4_... (flutter/engine#18576)
2020-05-22 [email protected] Roll Skia from 317dce5c81c0 to da90c3765908 (1 revision) (flutter/engine#18575)
2020-05-22 [email protected] Roll Dart SDK from 4ee57c08e0a6 to 65113fd73dec (3 revisions) (flutter/engine#18574)
2020-05-22 [email protected] Roll Skia from 67e21a19259b to 317dce5c81c0 (3 revisions) (flutter/engine#18573)
2020-05-22 [email protected] Update CanvasKit to 0.15.0 (flutter/engine#18570)
2020-05-21 [email protected] Roll Skia from 3d52abc84667 to 67e21a19259b (1 revision) (flutter/engine#18568)
2020-05-21 [email protected] Roll Dart SDK from 396b5fb7a97d to 4ee57c08e0a6 (6 revisions) (flutter/engine#18567)
2020-05-21 [email protected] Add fl_value_to_string (flutter/engine#18540)
2020-05-21 [email protected] Roll Skia from 1e63279156d6 to 3d52abc84667 (5 revisions) (flutter/engine#18566)
2020-05-21 [email protected] null-annotate semantics.dart (flutter/engine#18553)
2020-05-21 [email protected] Roll Fuchsia Linux SDK from ciqRH... to Kvwlr... (flutter/engine#18565)
2020-05-21 [email protected] Roll Skia from b37105ea6cca to 1e63279156d6 (9 revisions) (flutter/engine#18564)
2020-05-21 [email protected] Implement WriteAtomically using write/fsync on all platforms, and enable (flutter/engine#18320)
2020-05-21 [email protected] Roll Dart SDK from ecce58c1e354 to 396b5fb7a97d (2 revisions) (flutter/engine#18560)
2020-05-21 [email protected] Roll Fuchsia Mac SDK from RNByJ... to _zNmv... (flutter/engine#18558)
2020-05-21 [email protected] [profiling] Memory Profiling support for iOS (flutter/engine#18516)
2020-05-21 [email protected] Roll Skia from 22636205ce92 to b37105ea6cca (2 revisions) (flutter/engine#18557)
2020-05-21 [email protected] Roll Dart SDK from d551980ac9fc to ecce58c1e354 (2 revisions) (flutter/engine#18556)
2020-05-21 [email protected] Roll Dart SDK from bf7e9d13d730 to d551980ac9fc (6 revisions) (flutter/engine#18551)
2020-05-21 [email protected] Roll Skia from 4c0578632217 to 22636205ce92 (4 revisions) (flutter/engine#18550)
2020-05-21 [email protected] Roll Fuchsia Linux SDK from ThMeW... to ciqRH... (flutter/engine#18549)
2020-05-21 [email protected] Roll Skia from 67ff541ac116 to 4c0578632217 (1 revision) (flutter/engine#18547)
2020-05-21 [email protected] Let run_tests.py just stream output (flutter/engine#18534)
2020-05-21 [email protected] Add FlKeyEventPlugin (flutter/engine#18313)
2020-05-21 [email protected] Add tests for StandardMethodCodec (flutter/engine#18521)
2020-05-21 [email protected] Roll Skia from f83baf230c69 to 67ff541ac116 (1 revision) (flutter/engine#18545)
2020-05-21 [email protected] Roll Fuchsia Mac SDK from 3o9aQ... to RNByJ... (flutter/engine#18543)
2020-05-21 [email protected] fuchsia: Fix runtime_tests and shell_tests (flutter/engine#18492)
2020-05-21 [email protected] Roll Skia from d2dc8ddcdf5e to f83baf230c69 (2 revisions) (flutter/engine#18539)
2020-05-21 [email protected] Roll Dart SDK from 7aa8656d1dd8 to bf7e9d13d730 (21 revisions) (flutter/engine#18538)
2020-05-21 [email protected] Add FlMethodChannel, FlMethodCodec, FlStandardMethodCodec and FlJsonMethodCodec (flutter/engine#18220)
2020-05-20 [email protected] [web] Fix arc rendering when it starts a new sub path. (flutter/engine#18535)
2020-05-20 [email protected] Send platformResolvedLocale from iOS embedder (flutter/engine#18519)
2020-05-20 [email protected] System mouse cursor: macOS (flutter/engine#18131)
...
@liyuqian liyuqian mentioned this pull request May 26, 2020
Closed
@arbreng arbreng mentioned this pull request May 27, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jason-simmons jason-simmons Awaiting requested review from jason-simmons

1 more reviewer

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gw280 @chinmaygarde @googlebot @fluttergithubbot