Make file writes atomic, and don't decode empty file.

[mbrandonw]

There's a migration path from 0.x/1.x to 2.x where an empty file is on disk and the library tries to decode that. We can just resume with the default value when that happens.

[KaiOelfke]

This silences the issue that I can reproduce via upgrading Sharing versions. But I have some concerns:

• Shouldn’t the value on disk be replaced with the stub to eliminate the empty bytes as it’s no longer a valid case for recent Sharing versions?
• Shouldn’t this be a one time migration? If any other actual bug in the app code or library code causes empty bytes this will be largely ignored other than unit tests failing, if the initial value is not expected in a given test. It’s better to notice an issue during development. But now empty bytes won’t report an issue and one has to pay attention to whether something is supposed to have the initial value or something else.

There’s a discussion in Slack, whether there’s other potential reasons for empty bytes other than this migration. I still get a surprising amount of reported issue in my logging backend. I wonder, if these are really all old users opening an updated app version the first time after quite a while. I did expect the issue to decline faster as the time I was using a relevant version of Sharing was short. At least for the last 7 days the occurrence is actually declining so maybe it’s just slower than expected.

[pyrtsa]

I'd be more careful as well and not merge this PR as is, as it seems to only hide a potential issue of user data loss: A significant percentage of iOS users have built a habit of force-quitting their app, however unrecommendable it may be. While there may be nothing we can do to save their just changed data at app termination, the current logic can result in JSON file truncation equivalent of losing both the old and new copy of the user's data. I'd prefer at least seeing a runtime error the next time such a file is being read.

[mbrandonw]

Hi @KaiOelfke and @pyrtsa, we just pushed some updates that enable atomic writes. We are also keeping the empty bytes check for the migration path, but if that seems like a problem let us know what concerns you have. Thanks for helping us out with this!

[pyrtsa]

Would data.write(to: url, options: .atomic) work now?

[mbrandonw]

Oops, that was there but must have been dropped while cleaning up. Will bring back...

[pyrtsa]

Also thanks for using named arguments in these closures, makes code a lot easier to read. (With the currently limited support for column breakpoints, I wouldn't mind if newlines were sprinkled here and there as well, makes debugging a lot easier at times!)

[mbrandonw]

Haha sure thing. Just pushed some newlines. And the atomic write is now back up!

[mbrandonw]

BTW, for posterity, the reason that tests started failing with .atomic writes is that the dispatch source sees that has a delete event, not a write or rename.

[KaiOelfke]

We are also keeping the empty bytes check for the migration path, but if that seems like a problem let us know what concerns you have.

But the check is still active all the time and not just once during a migration so any other root cause other than migrating and atomic writes will still be silently ignored. If you think this is the best way to do it then go ahead. I'd have implemented it by writing some small versioning state on disk so I can determine, if a migration already happened and only if not I'd replace empty bytes with initial values.