SQLite3Adapter: Ensure fork-safety#52931
Conversation
|
This looks good to me (but I'd like to give it a closer look on Monday). |
| end | ||
| end | ||
|
|
||
| ActiveSupport::ForkTracker.before_fork { ActiveRecord::ConnectionAdapters::PoolConfig.prepare_to_fork! } |
There was a problem hiding this comment.
One issue here, is that after_fork is simple, because in the child, only the thread that forked survived.
But before_fork is much trickier, because nothing guarantees no thread was spawned, nor that any of these threads isn't using a connection right now.
So we potentially have a big thread safety on our hands here.
| # | ||
| # More on sqlite fork-safety: https://www.sqlite.org/howtocorrupt.html section 2.6 | ||
| # More on Rails db corruption: https://github.com/rails/solid_queue/issues/324 | ||
| disconnect! |
There was a problem hiding this comment.
What happens if this is called concurrently while another thread is using that connection? Is it synchronized?
There was a problem hiding this comment.
Hum, actually, since sqlite3-ruby doesn't release the GVL during queries I guess we're fine. But given people are asking for the GVL to be released...
|
No idea if related or just bad luck but... |
and backfill coverage for deregistering callbacks.
Also add ForkTracker.unregister_before_fork and rename ForkTracker.unregister to unregister_after_fork for consistency.
Sqlite itself is not fork-safe, as documented in https://www.sqlite.org/howtocorrupt.html section 2.6. Any databases that are inherited from the parent process are unsafe to properly close and may impact either the parent or the child and potentially lead to database corruption. In this commit, a pre-fork callback `prepare_to_fork!` is introduced for the connection pools and adapters to take any necessary actions before forking. The callback is only implemented by the SQLite3Adapter which now invokes `disconnect!` to close all open prepared statements and database connections. As a side effect, the parent process will end up having to re-open database connections if it continues to do work, which may be a small performance overhead for some use cases, but is necessary in order to prevent database corruption. See rails/solid_queue#324 for examples of the type of corruption that can occur.
flavorjones
force-pushed
the
flavorjones-sqlite3-fork-safety
branch
from
eadbfb7 to
c3e02fb
Compare
I believe this issue was fixed by sparklemotion/sqlite3-ruby#556 and will be in a new release shortly. |
|
This is not the right way to fix this -- forking should not affect the parent process's connections. We should be doing this:
in Edit: Thanks for identifying this! We (I) had obviously assumed it was safe to do regular closes across forks while teaching |
|
@matthewd Thanks for taking a look.
I appreciate this response, I do know this is an unexpected approach and I'm not entirely happy with it myself. I will take another look at closing the file descriptors in the child, however I'll note that I did explore this already and did not find a method supported by the sqlite C API to access the file descriptors (and there may be multiple per database, e.g. WAL mode) nor a way to close them. But, like I said, I'll take another look. |
|
Yeah, I'm afraid SQLite doesn't allow to just discard connections in the child:
https://www.sqlite.org/howtocorrupt.html Of course there's always the option to leak the connection, but... That said, perhaps this fork safety protection would be better located in the sqlite gem? |
|
Hum:
It's unclear how to get the file descriptor though. Perhaps: |
If I can figure out how to close the file descriptors (and can determine that's safe) then yes, I'd prefer to put it in the sqlite3-ruby gem.
Yes, thanks, I'm looking into how to use |
|
Sounds good. There's still a lot of unknowns though. Even if we close the FDs, we still need to free the allocated memory, and it's unclear how. Perhaps it's safe to call |
I can fix this in the sqlite3-ruby gem, it will be GCed and deallocated but won't call |
How do you know for sure sqlite doesn't need to free some other structs? Is everything kept in a single memory block? |
I'm going to take some time to look into it. Trust me! ♥ |
|
Oh, I trust you ❤️ , I'm just curious. |
It looks like Edit: I should be able to test the new |
|
I'm going to close this since the file descriptor gambit seems to be working. I will drop a pointer to the sqlite3-ruby pull request when I create it (likely tomorrow). And then I'll open a new PR that will implement Edit: work in progress is sparklemotion/sqlite3-ruby#558 |
|
@matthewd @byroot OK, so simply closing just the file descriptors (which is described as "safe") leaks quite a bit of memory from the improperly-closed connection inherited from the parent. So "safe" doesn't mean "optimal" or "recommended". This forum thread from last year deals with this question (in the context of a Lua app): https://sqlite.org/forum/forumpost/1fa07728204567a0a136f442cb1c59e3117da96898b7fa3290b0063ae7f6f012 and the advice seems to be, "either close all connections before the fork or be OK with memory leaks". Any advice or thoughts would be greatly appreciated. |
What happens if you IMO this PR wasn't too bad, closing before fork is way more annoying that after, but doable. The real issue is to ensure there isn't any active thread using the connections you are trying to close. |
|
I think closing the FDs and accepting the possibility of leakage puts us on par with the other adapters: from memory I think our usage may not be officially supported by pg / mysql2 either.
Setting aside the more-possible It's built-in rather than being something the user must configure, but that makes this a worse version of #31241 (which at least had the merit of only affecting a particular class of forks).
How much memory are we talking? As a much more heavy-handed approach, under the argument that a well-behaved C-ext will ideally defer to Ruby for allocations, perhaps there's an extreme option involving |
Not really, with pg/mysql we simply call close. Since the IO is a socket and not a file, there is no concens about locks and such on the file descriptor.
Seems way too extreme to me, and likely won't work if it does some malloc at library init time etc. |
Segfault.
On my Linux machine, it's
Maybe? I do appreciate the value of purpose-built arenas. But that feels risky and would take some time to determine if it can be done safely. Meanwhile we still have databases getting corrupted. I propose:
WDYT? |
|
I've written some of this up in a doc that, if we agree this is the best path forward, would be added to the sqlite3-ruby docs: sparklemotion/sqlite3-ruby#558 I'd love feedback. |
I think a warning is preferable yes. For the overwhelming majority of cases, that will only happen once because you generally just fork once, (or perhaps twice with Puma fork_worker), but for people using Pitchfork, or doing more exotic stuff, a warning would be appreciated as a way to say: figure yourself how to safely close all sqlite connections.
Honestly that sounds way too risky to me.
Your doc sound fair to me. I haven't check the implementation though. |
|
@matthewd Thanks for pushing back on this, I think we ended up with a much cleaner and more universal solution in sparklemotion/sqlite3-ruby#558. |
3 participants
Motivation / Background
Currently,
SQLite3Adapteris not fork-safe and can result in database file corruption when a Rails process forks under certain circumstances.rails/solid_queue#324 described sqlite database corruption happening while dealing with solid_queue job management. An investigation demonstrated that a likely cause of that corruption is sqlite's lack of fork-safety, a situation which was ironically made worse when the sqlite3-ruby driver improved its memory management with sparklemotion/sqlite3-ruby#392 in v2.0.0.
Any
SQLite3::Databaseobjects that are inherited from the parent process are unsafe to properly close and may impact either the parent or the child and potentially lead to database corruption.You can read more about sqlite's lack of fork-safety in How To Corrupt An SQLite Database File §2.6, and reproduce corruption yourself with https://github.com/flavorjones/2024-09-13-sqlite-corruption.
Detail
In this commit, a pre-fork callback
prepare_to_fork!is introduced for the connection pools and adapters to take any necessary actions before forking. The callback is only implemented bySQLite3Adapterwhich now invokesdisconnect!to close all open prepared statements and database connections.This pull request has a lot going on. The commits, in order, do the following:
activesupport/test/fork_tracker_test.rb.ActiveSupport::ForkTracker.before_forkfor registering callbacks before a Rails process forks.prepare_to_fork!callback andSQLite3Adapter's specialized version of that method.Additional information
As a side effect, the parent process will end up having to re-open database connections if it continues to do work, which may be a small performance overhead for some use cases, but is necessary in order to prevent database corruption.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]