Allow save even if writeToURL is invoked in a background thread.

[adib]

If the user chose to save anyway in response to a "The file has been changed by another application." prompt, the writeToURL… method may be invoked in a background thread.

[mikeabdullah]

Hi @adib, I'm surprised by this sequence of events. Do you have an example of how I might be able to reproduce it? I'd have thought the workflow goes:

1. On main thread: -saveToURL:ofType:forSaveOperation:delegate:didSaveSelector:contextInfo:
2. Prompt the user to see if they really want to save
3. -saveToURL:ofType:forSaveOperation:completionHandler:
4. On secondary thread: -writeSafelyToURL:ofType:forSaveOperation:error:
5. writeToURL:ofType:forSaveOperation:originalContentsURL:error:

Are you seeing something different then?

The main thing that worries about pinging back across to the main thread is it tends to deadlock since -unblockUserInteraction hasn't been called yet.

[adib]

I did manual core data store migration on configure persistent store coordinator and the "... modified by another application..." dialog shows. When I clicked Save on that dialog, it fails with an assertion failure and that's how the patch came to be.

After writing the patch, force-saving on that dialog works fine. Then I further suppress the dialog by calling setFileModificationDate with nil at just after migration completes.

Thanks

Sasmito Adibowo http://basilsalad.com +65 8135 8937

On 3 Jun, 2013, at 18:53, Mike Abdullah notifications@github.com wrote:

Hi @adib, I'm surprised by this sequence of events. Do you have an example of how I might be able to reproduce it? I'd have thought the workflow goes:

On main thread: -saveToURL:ofType:forSaveOperation:delegate:didSaveSelector:contextInfo: Prompt the user to see if they really want to save -saveToURL:ofType:forSaveOperation:completionHandler: On secondary thread: -writeSafelyToURL:ofType:forSaveOperation:error: writeToURL:ofType:forSaveOperation:originalContentsURL:error: Are you seeing something different then?

The main thing that worries about pinging back across to the main thread is it tends to deadlock since -unblockUserInteraction hasn't been called yet.

— Reply to this email directly or view it on GitHub.

[mikeabdullah]

Aha, here we go then. The workflow is actually:

1. On main thread: -saveToURL:ofType:forSaveOperation:completionHandler:
2. completionHandler is called with an error to say the save the document can't be saved as it's been edited
3. Error is presented to user
4. User chooses recovery option to Save Anyway
5. Document internals go about their work , calling: -writeSafelyToURL:ofType:forSaveOperation:error: on worker thread
6. writeToURL:ofType:forSaveOperation:originalContentsURL:error:

This poses a bit of a pickle. Some ideas:

• Call back across to the main thread, as your patch does, but this introduces a tiny race condition
• Call back across to the main thread, before having called -unblockUserInteraction, but this will deadlock
• Substitute in a custom error with our own recovery attempter that runs the content-gathering logic before continuing onto NSDocument's. Pretty fragile to the errors ever changing structure
• Re-implement -saveDocumentWithDelegate:didSaveSelector:contextInfo: and -autosaveDocumentWithDelegate:didAutosaveSelector:contextInfo: to do our own error presentation so we can know if the user chooses to recover. This also feels pretty fragile/dangerous to me
• In the event of an error, -saveToURL:ofType:forSaveOperation:completionHandler: could hang onto _content, ready for the save to proceed should the user agree

I think it's this last option I favour the most, but working in some of your changes too.

[mikeabdullah]

Aha, I've thought of a more graceful approach: 7f043447ecb9ed55d4cee9106fd200003bfb3fee

[adib]

Hi Mike,

At a glance the last option sounds feasible. However from my memory, I believe it was an autosave operation and didn't recall that saveToURL:ofType:forSaveOperation:completionHandler: was part of the call stack when it crashed. Then again I'll have to re-test it.

With best regards

Sasmito Adibowo Basil Salad Software Practicing your speech? +65 8135 8937

On 3 Jun, 2013, at 20:50 , Mike Abdullah notifications@github.com wrote:

Aha, here we go then. The workflow is actually:

On main thread: -saveToURL:ofType:forSaveOperation:completionHandler: completionHandler is called with an error to say the save the document can't be saved as it's been edited Error is presented to user User chooses recovery option to Save Anyway Document internals go about their work , calling: -writeSafelyToURL:ofType:forSaveOperation:error: on worker thread writeToURL:ofType:forSaveOperation:originalContentsURL:error: This poses a bit of a pickle. Some ideas:

Call back across to the main thread, as your patch does, but this introduces a tiny race condition Call back across to the main thread, before having called -unblockUserInteraction, but this will deadlock Substitute in a custom error with our own recovery attempter that runs the content-gathering logic before continuing onto NSDocument's. Pretty fragile to the errors ever changing structure Re-implement -saveDocumentWithDelegate:didSaveSelector:contextInfo: and -autosaveDocumentWithDelegate:didAutosaveSelector:contextInfo: to do our own error presentation so we can know if the user chooses to recover. This also feels pretty fragile/dangerous to me In the event of an error, -saveToURL:ofType:forSaveOperation:completionHandler: could hang onto _content, ready for the save to proceed should the user agree I think it's this last option I favour the most, but working in some of your changes too.

— Reply to this email directly or view it on GitHub.

[mikeabdullah]

Autosaves do go through -saveToURL:ofType:forSaveOperation:completionHandler: (or at least they're documented to!). It's the asynchronous nature of all this means the stack is often unhelpful.

[mikeabdullah]

Grr, there's some timing issues in my activity-based approach, so back to the simpler one I think

[mikeabdullah]

OK, give the latest duplicate-sqlite-stores branch a go. Does that fix the problem for you too?

[adib]

I'll need to resurrect the issue and get back to you to confirm this.

Thanks

Sasmito Adibowo http://basilsalad.com +65 8135 8937

On 3 Jun, 2013, at 22:24, Mike Abdullah notifications@github.com wrote:

OK, give the latest duplicate-sqlite-stores branch a go. Does that fix the problem for you too?

— Reply to this email directly or view it on GitHub.

[mikeabdullah]

Fair enough.

Big thanks from me for discovering this, BTW. I'd seen the symptoms of it in a couple of customer logs and was baffled!

[adib]

OK then. Bigger thanks from me for rewriting my initial implementation.

[adib]

Just did a test on 1189e6f – looks like you've nailed it.