Introduce {bool exclusive = false} parameter to File.create({recursive}) method

[aam]

Desktop OSs allow for a file to be created in exclusive mode enabling simple concurrency-serializing mechanism.

from https://man7.org/linux/man-pages/man2/open.2.html

       O_EXCL Ensure that this call creates the file: if this flag is
              specified in conjunction with O_CREAT, and pathname
              already exists, then open() fails with the error EEXIST.

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen

EEXIST | _O_CREAT and _O_EXCL flags specified, but filename already exists.

This currently is not available for Dart apps.
This can be fixed via new optional exclusive parameter to https://api.flutter.dev/flutter/dart-io/File/create.html:

Future<File>> create(
{bool recursive = false,
bool exclusive = false}
)

[a-siva]

//cc @itsjustkevin

[itsjustkevin]

Hey @aam please forgive my ignorance here, but is adding an additional parameter a breaking change if it has a default? There is usually a bit more context in these breaking change requests.

[aam]

@itsjustkevin yeah, unfortunately even adding optional parameter with default value is a breaking change since it will break all the code that implemented that class - potentially internal code that is used in testing, code that provided mock implementation of File class. Code that implemented File class with its own implementation of Future<File> create({bool recursive = false}) now has to be changed to Future<File> create({bool recursive = false, recursive = false})

[itsjustkevin]

Thanks for the breakdown @aam that was really helpful in understanding the scope of the change request!

Reaching out to @Hixie @grouma and @vsmenon to review this breaking change request.

[Hixie]

We should add exclusive to package:file before we add it here, to reduce the impact of the breaking change. Other than that, SGTM. cc @tvolkert

[aam]

We should add exclusive to package:file before we add it here,

Yeah, package:file is part of dart sdk build, so has to be addressed before File in dart:io dart sdk change can land.

[vsmenon]

We should study the breaking change impact, but lgtm.

[aam]

Submitted cl/468267059 to soften landing in g3.

[rrousselGit]

👋 I'm not against the addition. I don't find the name very clear though

exclusive is a bit too broad IMO and doesn't look related to "does the file exist"

What about excludeExist? create(excludeExist: true) would be more explicit I think

[aam]

exclusive was inspired by existing O_EXCL flag for open function in POSIX https://linux.die.net/man/3/open (that serves same function of having an open to fail when to-be-created already exists, and is actually used by the implementation).

[rrousselGit]

I understand that.
But I don't think someone could read create(exclusive: true) and understand its meaning unless they look at the documentation to know what exclusive does.

A name like excludeExist would still start with EXCL, so I think it would be a better compromise.

[aam]

Thank you for your input @rrousselGit , getting to a consensus on naming can be hard as space is vast.
excludeExist doesn't sound right to me as we are not excluding anything - the operation has to fail if file already exist.

It seems natural to me to interpret notion of exclusive file creation to mean that we are the only ones who can create a given file.

cc @lrhn for more thoughts.

[rrousselGit]

It seems natural to me to interpret notion of exclusive file creation to mean that we are the only ones who can create a given file.

Personally, at first glance I thought "exclusive" would be related to access rights.
For example "The author of the file created has exclusive rights to that file".

Even after learning about what this flag does, I still fail to see the relation between the flag name and the behavior.

But English isn't my natural language. So if that's natural to others, ignore me 😊

[lrhn]

Using the Posix flag name doesn't feel entirely right, because the API is not following the Posix structure otherwise (our open is not their open, we have create instead of open with O_CREAT.)

I see some possible alternatives:

• Make a separate method, like createNew, instead of putting a flag on create.
• Rename flag, say to mustNotExist or mustBeNew or newOnly. Not great names, I admit. Something shorter would be better, but new is a keyword.
• Don't just fail if the file exists, allow the use to do something, adding {void Function(FileSystemEntity) ifExists} which, if provided and the file exists, so O_EXCL would made open fail), calls that function instead of creating the file. Basically, pass O_EXCL to open and if it fails, call ifExists. (Probably a little too speculative).

Not sure what users will want, though. It does feel mostly like it should be a flag on create, but naming is hard.

[aam]

Thank you @lrhn . If I had to go for alternatives I would stay with the flag, yes, and pick mustBeNew as a name.
Given that exclusive was the one approved by folks and under way too, will ideally need one or more voices in next couple days to change from exclusive to mustBeNew.

[a-siva]

Another option is change the orginal behavior of create which I feel was counter intuitive
The method was called 'create' which would mean create a file and I would have expected it to return an error 'file already exists' if the file already exists instead the current behavior just returns the currently existing file (did not do any creation).

[Hixie]

My approval did not consider the actual name and semantics. I'm fine with whatever name or API approach is most idiomatic for Dart.

[aam]

Given no more feedback on naming, going to submit the change as originally requested with exclusive optional parameter.

[MaryaBelanger]

@aam Were the api docs updated? dart and flutter
Doesn't look like they are in production but I could be misunderstanding.
Nevermind, found it! https://dart-review.googlesource.com/c/sdk/+/255482/7/sdk/lib/io/file.dart#224

Edit: Is the practice here to wait until the change rolls out with the next stable to update the docs? Are the api docs changes in a CL somewhere, not linked to this issue?

[devoncarew]

@aam - just to confirm - this change will be in 2.19.0 stable?

[aam]

@devoncarew it's in latest beta(2.19.0-255.2.beta), so I assume yes.

[devoncarew]

OK, for posterity, it looks like the first dev sdk with this change is 2.19.0-136.0.dev.

We are seeing some failures w/ this change; I opened google/file.dart#204 to track them, but to paraphrase that issue, older versions of package:file fail when run on newer SDKs. We can't fix the underlying issue (retroactively adjust the supported sdk version range of the older package:file versions) but can likely mitigate things by publishing new versions of packages that use package:file.