Skip to content
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

Default amalgamation build should really include HAVE_USLEEP=1 #597

Closed
bengotow opened this issue Apr 15, 2021 · 2 comments
Closed

Default amalgamation build should really include HAVE_USLEEP=1 #597

bengotow opened this issue Apr 15, 2021 · 2 comments

Comments

@bengotow
Copy link
Contributor

bengotow commented Apr 15, 2021

Hey! I've been using the better-sqlite3 module in Mailspring (a cross platform Electron app) for about 6 years and this is the best sqlite adapter out there.

There's just one problem I have been patching with my own fork (and now via a custom amalgamation). Without HAVE_USLEEP=1 at compile time, if sqlite encounters lock contention it tries to aquire the lock every second instead of every 10 milliseconds, assuming that the system clock does not support usleep(usec) and only sleep(seconds). Letting this loose in your application can be super mysterious - it leads to transactions taking a weirdly long time to aquire, random "database is locked" timeouts, etc. In Electron apps it's particularly likely to happen, because each window of the Electron app is it's own NodeJS process. If two application windows try to write to the database at the same time, the second window won't retry for a full second and the query will take 1s+.

I /think/ that every platform that supports NodeJS also supports usleep (I've been safely setting it and shipping Mailspring for Ubuntu 14+ / linux distros, macOS and Windows 10!)

I would absolutely love if it could be part of the blessed library and I think it would likely make other people's apps faster if they haven't realized it needs to be turned on. Thanks for making (and maintaining) a great library!

@JoshuaWise
Copy link
Member

I tried adding this, but it caused the CI tests to fail due to sleeping for longer than expected. It worked fine on my local machine. I'm not sure why it was so slow in the CI workflow.

@JoshuaWise
Copy link
Member

Added in 6ee3399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants