fix: handle temp file removal race condition in concurrent initializa… #1185
Conversation
…tion When multiple SQLPage instances start simultaneously in the same directory, they can encounter a race condition during initialization. The create_default_database() function creates a temporary file to test directory writability, then removes it. If multiple instances try to remove the same file concurrently, some panic with 'No such file or directory'. This commit replaces the .expect() panic with graceful error handling using if let Err(). The writability test has already succeeded by the time we try to remove the file, so whether another instance removed it is irrelevant. Includes a test that spawns 10 concurrent threads initializing AppConfig to verify no panics occur. ref sqlpage#1183
| )); | ||
| std::fs::create_dir_all(&test_dir).unwrap(); | ||
|
|
||
| env::remove_var("DATABASE_URL"); |
There was a problem hiding this comment.
I don't think it's safe to just remove this env var like that. We are actually relying on this env var in other tests when running against a non-default db.
|
@mukhtaronif did you actually manage to reproduce the crash with the test ? I tried removing the fix but keeping the test, and it does not catch the problem for me. |
No, I wasn't able to reliably reproduce the crash with the test either. I also tried reverting the fix and running the test multiple times, and it consistently passed. The race condition is timing-dependent and difficult to trigger in a test environment. Even with a barrier to synchronize the threads in the test, the file system operations complete so quickly that the race window is extremely narrow.
|
|
We have decided to let surveilr set the DATABASE_URL environment variable before calling AppConfig::from_cli(), which prevents SQLPage from calling create_default_database() entirely and avoids triggering the race condition in the first place. |
|
ok, let's remove the test altogether then |
2 participants
…tion
When multiple SQLPage instances start simultaneously in the same directory, they can encounter a race condition during initialization. The create_default_database() function creates a temporary file to test directory writability, then removes it. If multiple instances try to remove the same file concurrently, some panic with 'No such file or directory'.
This commit replaces the .expect() panic with graceful error handling using if let Err(). The writability test has already succeeded by the time we try to remove the file, so whether another instance removed it is irrelevant.
Includes a test that spawns 10 concurrent threads initializing AppConfig to verify no panics occur.
ref #1183