-
Notifications
You must be signed in to change notification settings - Fork 59
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
TypeError: n.OpfsSAHPoolDb is not a constructor #79
Comments
To the best of my recollection, you are the first to report this particular problem, and no immediately obvious culprit comes to mind. Is it limited to a specific browser, or are you seeing this in at least two of Chrome, Firefox, and Safari? |
Thanks for the feedback @sgbeal. We're seeing this error in Chromium browsers including Google Chrome and Brave as well as both Desktop and Mobile Safari. There haven't been any reports of this error in Firefox, but our app only has about 100 active users at this point and, according to our logs, no one has used Firefox within the past month. I think we've been running into this error for 3+ months now and we haven't been able to recreate it consistently. Though at this point most of our users have experienced it at least once. When it happens, our best advice at the moment is to clear the browser cache, close all open tabs, and try reopening the application. Generally this works but not always. Notably, the error can be hard to get rid of on Mobile Safari on iOS. Edit: see my comment below for what is, I think, a more accurate description of the error we are seeing.Expand to see original text: Looking at the source code, the error seems related to this code inside `installOpfsSAHPoolVfs`...Based on the source code, this error could occur if // other code...
const poolUtil = new OpfsSAHPoolUtil(thePool);
if (sqlite3.oo1) {
const oo1 = sqlite3.oo1;
const theVfs = thePool.getVfs();
const OpfsSAHPoolDb = function (...args) {
const opt = oo1.DB.dbCtorHelper.normalizeArgs(...args);
opt.vfs = theVfs.$zName;
oo1.DB.dbCtorHelper.call(this, opt);
};
OpfsSAHPoolDb.prototype = Object.create(oo1.DB.prototype);
poolUtil.OpfsSAHPoolDb = OpfsSAHPoolDb;
oo1.DB.dbCtorHelper.setVfsPostOpenSql(
theVfs.pointer,
function (oo1Db, sqlite3) {
sqlite3.capi.sqlite3_exec(
oo1Db,
[
'pragma journal_mode=DELETE;',
'pragma cache_size=-16384;',
],
0,
0,
0,
);
},
);
}
thePool.log('VFS initialized.');
return poolUtil; |
I was able to recreate the error using the Desktop Safari browser. Anecdotally I've found that the error is easier to recreate on Safari (not easy to recreate as it still is seemingly triggered randomly). It looks like a better title for this issue is Is there a way to recover from this error? Here's a video showing the console output after the bug has occurred. https://www.loom.com/share/0a5034f69a0d448ab789b0f815bb60e8?sid=b4fc3b98-3c0e-4df8-8683-6afbd20c99c3 Here's the console log from Safari Console.txt. Unfortunately the formatting isn't great. The error details from the console
The stack trace for the error appears to be ![]() The methods referenced in the stack trace are class OpfsSAHPool {
#logImpl(level, ...args) {
if (this.#verbosity > level)
loggers[level](this.vfsName + ':', ...args);
}
error(...args) {
this.#logImpl(0, ...args);
}
storeErr(e, code) {
if (e) {
e.sqlite3Rc = code || capi.SQLITE_IOERR;
this.error(e);
}
this.$error = e;
return code;
}
async acquireAccessHandles(clearFiles) {
const files = [];
for await (const [name, h] of this.#dhOpaque) {
if ('file' === h.kind) {
files.push([name, h]);
}
}
return Promise.all(
files.map(async ([name, h]) => {
try {
const ah = await h.createSyncAccessHandle();
this.#mapSAHToName.set(ah, name);
if (clearFiles) {
ah.truncate(HEADER_OFFSET_DATA);
this.setAssociatedPath(ah, '', 0);
} else {
const path = this.getAssociatedPath(ah);
if (path) {
this.#mapFilenameToSAH.set(path, ah);
} else {
this.#availableSAH.add(ah);
}
}
} catch (e) {
this.storeErr(e); // <-- THE ERROR CAME FROM HERE
this.releaseAccessHandles();
throw e;
}
}),
);
}
} |
Digging farther, I couldn't find a reference to Edit
Looking at the File System Standard to learn what a "bucket file system" is (spec):
This explanation didn't mean much to me, but then I found a blog post with a better explanation for what a "bucket file system" is. And the blog post happened to be written by Thomas Steiner, the primary contributor to this repo, no less! Thanks for the great article Thomas!
So apparently "bucket file system" is an alias for the OPFS. Which seems to indicate that the browser thinks this file system handle isn't part of the OPFS, which seems like that has to be a browser bug? I'll also note that both times I've been able to trigger this locally today I've been using the Desktop Safari browser (though I've also personally triggered it several times on Google Chrome and Brave in the past) and the |
In the canonical builds, sqlite3.oo1 literally cannot be falsy. We ostensibly have the option of building without that API but no current builds do so. If it's falsy in your environment then something beyond my ken has gone horribly wrong. If it's falsy only sometimes then something even worse has gone even more horribly wrong. That "cannot happen" in a well-behaved environment.
Random browser-side hiccups have unfortunately proven to be a rare, difficult-to-trigger fact of life for which we have no specific cure, nor even so much as a rule of thumb for how to deal with them. The behavior you're reporting is outside the realm of the OPFS API contract, and therefore not a behavior the VFS is/can be prepared to deal with. Sidebar: one concrete example of such an issue is that some Chrome versions will not immediately release OPFS locks when a page is reloaded, so tapping Ctrl-R on an OPFS-using page may cause the reloaded instance to hit locking errors because the first one has not been fully disposed of when the reload happens. There's nothing we can do about such problems except to tell affected folks "reload once more and it will probably work."
My (mis?)understanding is that Bucket FS is a specific model for an FS and OPFS is one such FS (perhaps the only current one?). The term Bucket FS didn't enter public usage until long after we had published this VFS as the "OPFS VFS," so we (in the sqlite project) stick with the term OPFS. (It's like when Sun Microsystems decided, after releasing Java 1.2, to start calling it Java 2, it took many years for everyone else to start calling it that.) |
So we have run into this specific issue before and it was actually pretty easy to work around. We're initializing the sqlite3 module inside a const adapter = await promiseRetryOnError(
{ attempts: 2, waitMs: 5000 },
() =>
createPersistedClientDatabaseAdapter({
logger,
clearOnInit: data.clearOnInit,
}),
); In theory I would expect something similar to work for any browser hiccups like what we're running into here. But in this case it seems like the sqlite module enters a permanent error state that can't be recovered from. It seems like this might be a flaw in the way the sqlite3 module is constructed, and not an inevitable outcome of random browser bugs. But it's hard to say since I'm still not precisely sure of what's causing this. Thanks so much for the feedback btw :)
We've also found that, sometimes, the browser won't release a Web Lock held by a tab when the tab is closed or other times when a tab is reloaded. This happens disturbingly often in Safari. It's happened a few times in Google Chrome. In Chrome's case the browser eventually released the locks (e.g. after 30 seconds), but in Safari's case it generally won't release the locks when the problem occurs. No amount of reloading will help. The only solution for Safari is generally to close all open tabs/windows wait a few seconds, and then reopen. On desktop the solution has been to steer people away from Safari. On mobile we can direct them to a PWA or Native build which only uses a single tab and avoids the need for the web locks API. |
i was a unfortunately sloppy with my phrasing. My intent was that we cannot account for that type of quirk at the library level. Workarounds like that unfortunately fall into the realm of higher-level code. Like the C library, our JS API is synchronous (with the very unfortunate exception of some of the OPFS-related bits), which precludes library-level workarounds like wait-and-retry. If you can provide a case which reproduces the problem, even if only periodically, i'll be happy to look at it but can make no promises when it comes to this type of strangeness. After having looked at @jorroll's post and video: According to: https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileHandle/createSyncAccessHandle
The implication of that is that the file handle from which we are attempting to extract the sync access handle (we have to have the former to get the latter) is being pulled out from under us by some |
Ah HA! I believe I have a fix of sorts (really, a work-around)!! So as previously discovered, the problem we're seeing is that, sometimes (seemingly randomly), an class OpfsSAHPool {
async acquireAccessHandles(clearFiles) {
const files = [];
for await (const [name, h] of this.#dhOpaque) {
if ('file' === h.kind) {
files.push([name, h]);
}
}
return Promise.all(
files.map(async ([name, h]) => {
try {
const ah = await h.createSyncAccessHandle(); // <-- Error is being thrown here
this.#mapSAHToName.set(ah, name);
// code continues... We already try to simply re-run the initialization logic if there's an unhandled error (as mentioned above we've found this addresses some browser bugs), but that hasn't helped address this issue. Looks like the reason why is that sqlite3.installOpfsSAHPoolVfs = async function (
options = Object.create(null),
) {
const vfsName = options.name || optionDefaults.name;
if (0 && 2 === ++instanceCounter) {
throw new Error("Just testing rejection.");
}
if (initPromises[vfsName]) {
return initPromises[vfsName]; // <-- the problem is that we're not rerunning the init process here
}
// ...
return (initPromises[vfsName] = apiVersionCheck().then(async function () {
// code continues... So I tried forking this library and removing the A small, unfortunate side effect is that, because of the error during the install process, The big headline for us is that we don't need to ask the user to try reloading the page 🙌. I imagine that the Would this repo consider an update to either remove the Edit: nevermind, looks like this second issue is already fixed on Identifying this bug was made harder because of this line in the `sqlite3.installOpfsSAHPoolVfs()` source .catch(async (e) => {
await thePool.removeVfs().catch(() => { });
return e; // <--
}); Note that the error is being returned rather than thrown. This was surprising for us and is why the error was being surfaced as
Gotcha. Though since the error is being thrown during the initialization process, which happens in javascript and is already async, in theory it should be possible to catch some of these errors and retry after a delay. |
That's very intentional to avoid undefined behavior if the init is run twice. Removing that memo will eventually cause someone grief. The interface specifically supports clients calling it subsequent times in order to fetch the associated PoolUtil object, in case, e.g., the shape of their API does not enable them to get a reference to it via a post-init
Even if removing the memo would be a good idea (though i opine that it's not!), this project's strict backwards-compatibility policy would require us to keep it. What we can do, though, is...
with a big fat doc caveat stating users should never actually do that.
It's not so much about trusting the developer to not call it twice, but to enable them to call it any number of times with well-defined results. The motivating concept was to enable users at all levels of the API, from those working directly with the library to those using a higher-level application, to be able to get ahold of the relevant PoolUtil object or, if init failed, to be able to tell that init failed (by getting the cached failed promise back). The inability to double-init the VFS for a given slice of storage (i.e. a given VFS name) is very intentional: if the environment fails to initialize the VFS, the library has to assume that the environment is borked. There is no sane environment which will sometimes initialize and sometimes mysteriously not.
The problem is, as you allude to, that we don't know which of those "certain errors" we should be doing that for and which ones not, and we don't know what future mysterious errors may crop up. In the case of init failure, we need to (A) assume that the environment is unfit for our purpose and (B) leave the VFS's storage in a known state, and if we don't clean up then we're leaving it in an undefined state. In any case, here are the preliminary docs for the new flag, with the bold part denoting a difference from your initial proposal:
The intent of the bolded part is to reduce the "blast radius" of this nuclear option. i'm open to debating that part of its description if you like (and renaming the flag if that part is changed or removed). It's intentionally a bit vague on when it should be used (A) to dissuade folks from using it and (B) to future-proof it against similar future cases which may come up. Does that sound like a viable approach to you? |
He's a preliminary patch for the part of if(initPromises[vfsName]){
const p = initPromises[vfsName];
if( (p instanceof OpfsSAHPool) || !options.forceReinitIfFailed ){
return p;
}
delete initPromises[vfsName];
/* Fall through and try again. */
} It occurs to me now that this may introduce a new race condition when the new flag is used and the installation is run twice concurrently, but i'm sorely tempted to hand-wave that away as "if this flag is used, all bets are off." |
The prerelease snapshot has been updated with this change, which is currently in a branch pending buy-in and/or change suggestions from those involved in this discussion. The new flag's name is currently |
…ails on a first attempt, as a workaround for flaky environments where initial access to OPFS sync access handles is rejected but then permitted on a second attempt. Reported and discussed in [https://github.com/sqlite/sqlite-wasm/issues/79|issue #79 of the npm distribution]. FossilOrigin-Name: 5286e0f654d91a4ebee51fcabaab696e17ff07bb18990b401a31bd3d1213e695
In general, yes :). Though I think there's an issue with the current implementation.
Just looking at the implementation snippet you shared, I don't think this will work because Maybe something like the following: if (initPromises[vfsName]) {
try {
// `await` is necessary for errors to be caught here
return await initPromises[vfsName];
} catch (e) {
if (options.forceReinitIfFailed) {
/* Fall through and try again in this case. */
delete initPromises[vfsName];
} else {
throw e;
}
}
} I believe the above fix also addresses the race condition.
This makes sense 👍
Seems like a sensible approach 👍. |
Edit: correction - one of the tests was flawed and correcting it reveals the bug you mentioned. Even so, i agree that your approach looks like it would alleviate the potential race condition which i suspect (but am not certain) exists in the current impl, so it will be reformulated along those lines. i will post a follow-up and a new snapshot build when this is done, but it will probably be tomorrow (Friday). Thank you for taking a critical look at it! |
Okay, that went quickly. There's a new prerelease snapshot with the above-discussed changes, and the tests look to be more correct this time around. Please try that out and let us know if that will suffice for you (and whether i've made any boneheaded mistakes). Again, thank you for taking the time to test this out and provide feedback! Edit: for completeness's sake, the corresponding checkin is: https://sqlite.org/src/info/c4f468309158f9b9 |
Wow, thanks so much for all your incredibly fast support with this 🙏 !!! At first glance this prerelease snapshot looks good. Will test it out and report back! |
…ails on a first attempt, as a workaround for flaky environments where initial access to OPFS sync access handles is rejected but then permitted on a second attempt. Reported and discussed in [https://github.com/sqlite/sqlite-wasm/issues/79|issue #79 of the npm distribution].
i have one concern about the name Do we have any better name suggestions for |
@sgbeal what about |
Ya, I could see that confusion with the name. I also agree that it should be up to the library user to decide if/when to retry the attempt. For example in our case we'll be looking at the error and retrying some errors but not others. Other suggestions might be
FWIW I agree. |
That's both suitably descriptive and suitably onerous :). This change is now in the upstream trunk and will be available as of the 3.47 release. The prerelease snapshot page has been updated as well. Thanks to all of you who contributed to this resolution. It's not a 100% satisfying one, but it's no less satisfying than having to deal with environments which make such a workaround necessary :/. |
…ious init failure and retry, as an opt-in workaround for a browser quirk which occasionally denies OPFS access on the first attempt and permits it on subsequent attempts. This resolves [https://github.com/sqlite/sqlite-wasm/issues/79|issue #79 of the npm distribution]. FossilOrigin-Name: fbf3948a4ba27c6ebf55b24e7463b20026439f04d1974bafe78df5c5bc958f59
BTW: that part will need to be done by someone who contributes directly to the npm-related pieces. Within the upstream project we very willfully remain blissfully ignorant of any of the non-vanilla-JS components. @tomayac would most certainly appreciate a PR to that end, if you feel inclined to commit one. |
…ious init failure and retry, as an opt-in workaround for a browser quirk which occasionally denies OPFS access on the first attempt and permits it on subsequent attempts. This resolves [https://github.com/sqlite/sqlite-wasm/issues/79|issue #79 of the npm distribution].
To follow up, we've been using this fix in production for months now and it works 👍 |
If you would open a quick PR, I'd happily merge it. Thanks for considering! |
We're still using a local fork that contains the fixes described in this issue. My impression was that @sgbeal had initiated a process to include this fix in the upstream process, but it's still unclear to me if that process was completed. E.g. looking at the releases I don't see a mention of this fix. I have the impression that the fix itself needs to be done by a maintainer and that the actual canonical source code is somewhere upstream and not in this repo? Let me know if I'm mistaken @tomayac. I'm happy to add a PR for typing support but that obv needs to wait until after the fix itself has been merged. |
Sounds good, lets await news from @sgbeal then. Thank you! |
That change went into the trunk on July 13th: https://sqlite.org/src/info/fbf3948a4ba27c6e |
Hi, we are getting this error intermittently and we are having a really hard time recreating it and diagnosing what's happening. Does anyone here have an idea what's going on or have a suggestion of how we can debug it.
When the error occurs it prevents our app from initializing.
The text was updated successfully, but these errors were encountered: