Skip to content
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.

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

Windows.Storage: CreateFile(..., CreationCollisionOption.OpenIfExists) fails on non-existing file #477

Closed
Hassmann opened this issue Apr 29, 2019 · 10 comments · Fixed by nanoframework/nf-interpreter#1309

Comments

@Hassmann
Copy link

Details about Problem

nanoFramework area: Windows.Storage

VS version: VS 2017

VS extension version: 1.0.3.137

Target: ESP32

Firmware image version: 1.0.4-preview.416

Device capabilities output:
Native Assemblies:
mscorlib v1.1.1.7, checksum 0x7CCFBEC1
nanoFramework.Runtime.Native v1.0.4.4, checksum 0xFE14C7A7
nanoFramework.Hardware.Esp32 v1.0.6.0, checksum 0xF8079179
nanoFramework.Networking.Sntp v1.0.4.3, checksum 0x733B4551
nanoFramework.Runtime.Events v1.0.5.0, checksum 0xBFF88292
EventSink v1.0.0.0, checksum 0xF32F4C3E
System.Math v1.0.4.3, checksum 0x4BDCF00F
System.Net v1.0.6.0, checksum 0x808BAB84
Windows.Devices.Adc v1.1.3.1, checksum 0xE5D11571
Windows.Devices.Gpio v1.1.0.0, checksum 0xB1F30A6A
Windows.Devices.I2c v1.1.3.1, checksum 0xA44C698B
Windows.Devices.Pwm v1.1.3.1, checksum 0x75280B26
Windows.Devices.SerialCommunication v1.1.0.0, checksum 0xA8A5A0E0
Windows.Devices.Spi v1.1.3.0, checksum 0x083DBB79
Windows.Devices.Wifi v1.0.6.0, checksum 0x1B4DDB17
Windows.Storage v1.0.0.0, checksum 0x3BE02CF0

Detailed repro steps so we can see the same problem

On an internal storage device of ESP32,
try to create a new file with CreationCollisionOption.OpenIfExists

The call will fail with this output:

#### Exception System.Exception - CLR_E_FILE_NOT_FOUND (1) ####
#### Message: 
#### Windows.Storage.StorageFolder::CreateFileNative [IP: 0000] ####
#### Windows.Storage.StorageFolder::CreateFile [IP: 0006] ####
#### StorageIssue.Program::OpenFile [IP: 0014] ####
#### StorageIssue.Program::OpenOrCreate [IP: 0009] ####
#### StorageIssue.Program::Main [IP: 0004] ####

Exception thrown: 'System.Exception' in Windows.Storage.dll

Sample Project (if applicable)

StorageIssue.zip

@josesimoes
Copy link
Member

@Hassmann isn't this the expected behaviour?
With the above you're requesting to open the file if it exists. If it doesn't it will throw an exception because the file with that name does not exist.

Have you tested this code bit in a desktop console app for example?

@Hassmann
Copy link
Author

Hassmann commented Apr 29, 2019

@josesimoes: Actually, the meaning of this option is:
Create a new file, but if it already exists, open the existing one.

Windows.Storage in an UWP app is behaving this way.
Confirmed this.

@josesimoes
Copy link
Member

Roger that! Thanks for clarifying. 😉

@AdrianSoundy
Copy link
Member

AdrianSoundy commented May 2, 2019

Will look at this for Esp32 but need to understand how it meant to work on UWP.
Questions like:-
If you use CreationCollisionOption.OpenIfExists and file exists with data, does a write to that file append to existing data ?

@AdrianSoundy AdrianSoundy self-assigned this May 2, 2019
@Hassmann
Copy link
Author

Hassmann commented May 2, 2019

I know, it's a little awkward.
If you write to a file "created" with OpenIfExists via FileIO, it starts at the beginning of the file and will truncate the file to the written size, similar to ReplaceExisting. The use case for OpenIfExists is rather to read existing data.

If you want to append, there are two methods in FileIO:
AppendLines and AppendText.

Unfortunately, they work on strings only.

The correct way to append binary data to a file goes a little deeper.
nanoframework assumes the returned file from StorageFolder.CreateFile to be somehow open for read/write like in the good old days. But it's different...

In UWP you use this file handle to actually open it with another call. It looks like this (pseudo):

file = StorageFolder.CreateFile(OpenIfExists)
stream = file.Open(AccessMode.Write)
outputStream = stream.GetOutputStreamAt(stream.Size)

Writing to this outputStream will then append data.

The whole API is like this because of the highly asynchronous model behind UWP devices.

Possibly, trying to mimic the Windows.Storage namespace in nano might be the wrong path, anyway.
I could imagine that setting up a simple synchronous API for file handling, independent from UWP, would serve the cause better.

@josesimoes
Copy link
Member

Awkward feels kind of short for this... 😁

If one needs to read a full page of documentation and go throw a code sample just to understand how that works, to me, it's a clear indication of bad design.

This is probably one of those situations that we need to deviate from the UWP API.
A couple of ideas for discussion:

  1. We could drop the CreateFile() with options. Probably the most simple approach and considering the usage scenarios (embedded systems) it's quite enough.
  2. We could validate the option parameter and accept only FailIfExists (which is already the default) and ReplaceExisting. All the others throw a not supported exception.

@Hassmann
Copy link
Author

Hassmann commented May 3, 2019

There are good reasons for the design of the UWP API, but they do not apply to our kind of devices.
My suggestion, although it hurts, would be to drop Windows.Storage altogether and implement
the File class of System.IO in .NET Framework instead.
That's a well-defined synchronous API, it covers all kind of needs and might be even more straightforward to port.

@josesimoes
Copy link
Member

@Hassmann we currently have most of the stuff available in "old" System.IO available in Windows.Storage:

  • ReadBuffer
  • ReadText
  • WriteBuffer
  • WriteText
  • WriteBytes
  • Delete
  • GetFileFromPath
  • Rename
  • CreateFile
  • CreateFolder
  • GetFiles
  • GetFolders

Apart from the "async" suffix on several methods (which we don't have for know reasons) and the namespace there isn't much difference. The bulk of the work is done and most of the "missing" methods can be easily added without requiring any huge effort.

Because the UWP API is using StorageStream instead of the "old" Streams - which we don't have - it would be a pain to give that step back and move this to System.IO API.

@Hassmann
Copy link
Author

Yeah, I understand and get along quite well with the existing API.
It's just that an API that was designed for async cannot be simply de-asynced without creating repercussion.
And I think there shouldn't be Windows namespaces in this project. But I also think I should do it myself if it is important enough for me. Where is the time...

@AdrianSoundy
Copy link
Member

AdrianSoundy commented May 12, 2019

I already have a nanoFramework managed code assembly for System.IO with added XML comments which i did a while ago if you want to go that way. This Windows.Storage API is not a very intuitive and the System.IO is still a current namespace. If we continue then maybe we should rename it to nanoFramework.Storage. Could always support both namespaces. :-)

You need the OpenIfExisting parameter for CreateFile so you don't truncate existing data to allow you to call the FileIO.AppendText. Then we don't have appendText yet

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

Successfully merging a pull request may close this issue.

3 participants