A note on the new NSPersistentContainer code

[evanmiller]

I hit a nasty bug with the new code deployed to production. It's not a bug in the document class per se, but how I was using it, so I wanted to share a description of the problem. Maybe we can offer up some warnings in similar situations.

Symptom: Files that were not the primary document type would open intermittently. To handle secondary types (which are "opened" but immediately imported into the primary document type), I have code like:

- (BOOL)readFromURL:(NSURL *)absoluteURL ofType:(NSString *)typeName error:(NSError **)outError {
    if (![typeName isEqualToString:MyPrimaryDocumentType]) {
        [self initWithType:MyPrimaryDocumentType error:outError];
        self.fileURL = nil;
        _initial_file = [absoluteURL retain];
        return YES;
    }
    return [super readFromURL:absoluteURL ofType:typeName error:outError];
}

Later, after an empty document was initialized, _initial_file (e.g. a plain-text file) was imported into that document.

While the old code worked fine, the new NSPersistentContainer code resulted in a hang. The issue turned out to be that the forced autosave that occurs during context initialization noticed that fileURL was set (to the value of absoluteURL above), and attempted to autosave to that, even though it was an incompatible document type. But the caller to readFromURL:ofType:error: was already accessing the file, resulting in a blocked thread.

After much hair-pulling, the solution turned out to be straightforward: nil out the fileURL before the call to initWithType:error:. In the pre-NSPersistentContainer days, the document initialization occurred later, so it was fine to nil out fileURL after initWithType:error:.

The new code looks like:

- (BOOL)readFromURL:(NSURL *)absoluteURL ofType:(NSString *)typeName error:(NSError **)outError {
    if (![typeName isEqualToString:MyPrimaryDocumentType]) {
        self.fileURL = nil;
        [self initWithType:MyPrimaryDocumentType error:outError];
        _initial_file = [absoluteURL retain];
        return YES;
    }
    return [super readFromURL:absoluteURL ofType:typeName error:outError];
}

And peace has been restored to the valley.

[jerrykrinock]

Thank you, Evan. I think we should add a section of the README.md, but I would remove any references to the “old” code, since that will become less and less relevant, and just increase cognitive overhead for new adopters, as years go by. The ship has sailed, for good reasons, on NSPersistentContainer and the forced autosave. We just need to advise adopters how to deal with it.

If I understand this issue correctly, I think the title of this new section should be maybe something like, “Consideration when migrating one document type to another document type upon opening”. Its text would be maybe only 2 sentences, in the form:

If you want to do XXX, you must set the property fileURL to nil before initializing the other document. To learn why, refer to Issue 21.

[evanmiller]

I'm starting to think some kind of warning or error condition wouldn't be a bad idea. E.g. if fileURL is set and the file exists before the first autosave, make sure it looks like something that's not going to deadlock. I'll have to think it over.

[malhal]

I was wondering, does NSPersistentDocument use a context and a coordinator without a store loaded? If so, that means the objects in a new document are all in memory. This could be simulated with NSPersistentContainer by loading a store description with url "/dev/null" when creating a new document. Then on save it could be moved to the fileURL using setURL:forPersistentStore:, i.e. the same as the setFileURLoverride does when a file is moved in Finder. Would need to test if it is possible to move from the memory url to real url though.

Since "/dev/null" is a real sqlite store in memory, and context/coordinator with no store is just the objects, fetching might behave differently, e.g. if the UI has any sorting or filtering features that could be used on the new (and unsaved) document.