Fix thread-safety in initPageSize() using double-checked locking with static lock #1396
Conversation
Co-authored-by: anod <171704+anod@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Co-authored-by: anod <171704+anod@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the initPageSize() method where concurrent calls could execute the PRAGMA page_size query multiple times. The fix implements the double-checked locking pattern to ensure thread-safe lazy initialization while maintaining read performance.
- Made
m_pageSizefield volatile for proper cross-thread visibility - Added synchronized block with double-checked locking in
initPageSize() - Reorganized exception handling to work correctly within the synchronized block
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| private void initPageSize() { | ||
| if (m_pageSize == -1) { | ||
| try { | ||
| try (Cursor c = m_db.query("PRAGMA page_size", null)) { | ||
| if (c.getCount() == 1 && c.getColumnCount() == 1) { | ||
| c.moveToFirst(); | ||
| m_pageSize = c.getLong(0); | ||
| } else { | ||
| synchronized (PAGE_SIZE_LOCK) { | ||
| if (m_pageSize == -1) { | ||
| try { | ||
| try (Cursor c = m_db.query("PRAGMA page_size", null)) { | ||
| if (c.getCount() == 1 && c.getColumnCount() == 1) { | ||
| c.moveToFirst(); | ||
| m_pageSize = c.getLong(0); | ||
| } else { | ||
| m_pageSize = PAGE_SIZE_DEFAULT; | ||
| Log.e("MAE", | ||
| String.format("Unexpected result from PRAGMA page_size: %d rows, %d columns", | ||
| c.getCount(), | ||
| c.getColumnCount())); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| m_pageSize = PAGE_SIZE_DEFAULT; | ||
| Log.e("MAE", | ||
| String.format("Unexpected result from PRAGMA page_size: %d rows, %d columns", | ||
| c.getCount(), | ||
| c.getColumnCount())); | ||
| Log.e("MAE", "Failed to query PRAGMA page_size, using default page size.", e); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| m_pageSize = PAGE_SIZE_DEFAULT; | ||
| Log.e("MAE", "Failed to query PRAGMA page_size, using default page size.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The thread-safety fix for concurrent initialization lacks test coverage. Consider adding a test that verifies thread-safe initialization by calling totalSize() or loadPageSize() concurrently from multiple threads to ensure the PRAGMA query is only executed once and that no race conditions occur.
There was a problem hiding this comment.
Added ConcurrentPageSizeInitialization test that verifies thread-safe initialization by:
- Creating 10 threads that simultaneously call
loadPageSize()andtotalSize() - Using CountDownLatch to ensure concurrent execution
- Verifying all threads receive the same page size value (no race condition)
- Validating no errors occur during concurrent access
- Confirming page size is valid (positive and power of 2)
Changes in commit b140ab0.
Changed to use a static Regarding point 1: The query remains inside the synchronized block to ensure it runs only once. The double-checked locking pattern ensures that after Changes in commit b28b8fb. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: anod <171704+anod@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
The
initPageSize()method had a race condition where concurrent calls tototalSize()orloadPageSize()could execute the PRAGMA query multiple times.Changes
m_pageSizefieldvolatileto ensure visibility across threadsinitPageSize()thisConcurrentPageSizeInitializationtest to verify thread-safe initializationThe pattern ensures single initialization under concurrent access while maintaining read performance after initialization. Using a static lock object provides better encapsulation and prevents external code from interfering with the synchronization mechanism.
Test Coverage
The new
ConcurrentPageSizeInitializationtest verifies thread-safe initialization by:loadPageSize()andtotalSize()✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.