Add mount option { autoPersist: true } to IDBFS mount.

[juj]

Add new setting -sIDBFS_AUTO_PERSIST which changes the semantics of an IndedexDB mount to automatically persist the VFS to IndexedDB after closing any file that has been written to. This enables users to avoid needing to call FS.syncfs() or fsync() to persist files to the filesystem.

[curiousdannii]

Great idea! I was just adding this to my project (overwriting the default library function), but your way should be much more reliable.

    // Sync IDBFS after saving a savefile
    fd_close(fd) {
        try {
            const stream = SYSCALLS.getStreamFromFD(fd)
            const flags = stream.flags
            const path = stream.path
            FS.close(stream)
            // Don't run if we're currently syncing (because syncing will run this code)
            if (FS.syncFSRequests === 0) {
                // Then check that we're saving a savefile
                if (path.startsWith('/saves/') && flags & 3) {
                    FS.syncfs(err => {
                        if (err) {
                            console.log(err)
                        }
                    })
                }
            }
            return 0
        } catch (e) {
            if (typeof FS == 'undefined' || !(e.name === 'ErrnoError')) throw e
            return e.errno
        }
    },

[sbc100]

What do you think about enabling this by default?

[juj]

What do you think about enabling this by default?

Personally that would be ok for me, although I do worry that there might be some user projects that would be broken by such a change, in case they might have custom semantics for when the IndexedDB database would be synchronized. So maybe err to side of caution for now?

[sbc100]

What do you think about enabling this by default?

Personally that would be ok for me, although I do worry that there might be some user projects that would be broken by such a change, in case they might have custom semantics for when the IndexedDB database would be synchronized. So maybe err to side of caution for now?

sgtm. We can always consider enabling by default later.

[juj]

Any more reviews, or a ✅ ?

[sbc100]

What do you think about making this a mount option instead of a settings? (That would limit the scope of this chance to just this library file). Just an idea. I guess the downside would be a minor code size increase for idbfs users who don't need this feature, but I imagine it would be marginal.. since the FS code is so heavy.

[juj]

Yeah, the FS code is quite heavy already, so I wouldn't include it in the aggressive minimal code size optimization domain.

I suppose it could be an option, I'll see about refactoring it to that form.

[sbc100]

Can you write these as comma expressions to avoid the return statement and curly braces?

 mnt.node_ops.mkdir   = (...args) =>  IDBFS.queuePersist(mnt.mount), memfs_node_ops.mkdir(...args);

[juj]

Hmm, does that parse?

function f() { return 1; }
function g() { return 2; }
var foo = (...args) => f(), g();

gives a syntax error, though

function f() { return 1; }
function g() { return 2; }
var foo = (...args) => { return f(), g() };

works.

[sbc100]

Oh interesting. Looks like simple comma expression work: var foo = (...args) => f, g;.

More complex ones require braces I guess: var foo = (...args) => (f(), g());

[tlively]

Would we be able to make this the default behavior rather than adding another setting?

Edit: already discussed.

[juj]

Updated code to make this a runtime setting.

[sbc100]

lgtm. Looks like the PR title needs updating.

[sbc100]

Might be worth a ChangeLog entry?

I do think we should consider making this the default at some point. Perhaps we should reach out to existing users via the mailing list to see if this makes sense?

One possible way to make this the default and have folks opt out would be to have the public IDBFS.syncfs() assert if you its called when autoPersist is enabled?