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

fix(sqlite) Do not drop notify mutex guard until after condvar is triggered #2573

Merged

Conversation

andrewwhitehead
Copy link
Contributor

I believe this resolves an EXC_BAD_ACCESS error I've been seeing on MacOS under high contention.

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
@andrewwhitehead andrewwhitehead changed the title fix(sqlite) Do not drop notify mutex until after condvar is triggered fix(sqlite) Do not drop notify mutex guard until after condvar is triggered Jun 30, 2023
@abonander
Copy link
Collaborator

I can't find any documentation or bug reports that corroborate this, but I've come across multiple results that imply that you should signal the condvar before releasing the mutex, including SQLite's own documentation: https://www.sqlite.org/unlock_notify.html

It appears Apple hasn't updated the manpage for pthread_cond_signal() since they stole copied the implementation from OpenBSD, as it says absolutely nothing about this:

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/pthread_cond_signal.3.html

https://man.openbsd.org/pthread_cond_signal.3

Funnily enough, Linux's documentation explicitly says you don't need to own the mutex to call this function, which suggests to me they ran into the same issue at one point and just, you know, fixed it: https://linux.die.net/man/3/pthread_cond_signal

I've got a release to get out so I'm trying to avoid getting even more nerd-sniped by this, but uh, yeah. Weird stuff.

@abonander abonander merged commit 3fbc3a3 into launchbadge:main Jun 30, 2023
@andrewwhitehead
Copy link
Contributor Author

Thanks!

@andrewwhitehead andrewwhitehead deleted the fix/sqlite-unlock-notify branch July 1, 2023 01:28
@andrewwhitehead
Copy link
Contributor Author

BTW, I believe the main issue is that the waiting thread gets woken - somehow - and is able to acquire the mutex. It sees the true value and returns, dropping the Notify object. Then the callback thread tries to notify the condvar which has already been freed. The stack looks like this (on 0.6):

Thread 27 Crashed:: sqlx-sqlite-worker-37
0   libsystem_pthread.dylib       	       0x19e346f88 _pthread_cond_updateval + 180
1   libsystem_pthread.dylib       	       0x19e3492e8 pthread_cond_signal + 748
2   backends-6da39682e679ba4c     	       0x100e7f2a0 sqlx_core::sqlite::statement::unlock_notify::Notify::fire::hd378c69a7af0125d + 136
3   backends-6da39682e679ba4c     	       0x100e7f0f0 sqlx_core::sqlite::statement::unlock_notify::unlock_notify_cb::h333ff027081e3732 + 156
4   backends-6da39682e679ba4c     	       0x101053b58 sqlite3ConnectionUnlocked + 604
5   backends-6da39682e679ba4c     	       0x1010529ec sqlite3VdbeHalt + 1676
6   backends-6da39682e679ba4c     	       0x10105c03c sqlite3VdbeExec + 16456
7   backends-6da39682e679ba4c     	       0x101022554 sqlite3Step + 536
8   backends-6da39682e679ba4c     	       0x10101d5f4 sqlite3_step + 112
9   backends-6da39682e679ba4c     	       0x1010277d8 sqlite3_exec + 288
10  backends-6da39682e679ba4c     	       0x100ea430c sqlx_core::sqlite::connection::handle::ConnectionHandle::exec::hc6fcc29a48c860a7 + 436
11  backends-6da39682e679ba4c     	       0x100e566b8 sqlx_core::sqlite::connection::worker::ConnectionWorker::establish::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h7d5967ba2553f4b8 + 3944

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

Successfully merging this pull request may close these issues.

2 participants