Skip to content

Commit 11eeb58

Browse files
committed
fix queue processing deadlock
reported here: TryGhost#1838
1 parent 5034be3 commit 11eeb58

4 files changed

Lines changed: 142 additions & 3 deletions

File tree

memory-bank/decisionLog.md

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,46 @@
22

33
## Technical Decisions
44

5+
### 2026-04-10: Queue Processing Deadlock Fix
6+
7+
**Decision**: Track `pending` counter to detect synchronous operations in `Database::Process()`
8+
9+
**Rationale**:
10+
- Bug: When using `db.serialize()`, synchronous operations like `configure()` would cause subsequent operations to get stuck in the queue indefinitely
11+
- Root cause: `Process()` loop breaks after exclusive operations, but synchronous operations don't increment `pending`, leaving `locked=true` with no async work pending
12+
- Solution detects synchronous completion by checking if `pending` counter changed during callback execution
13+
14+
**Implementation**:
15+
```cpp
16+
// Track pending before callback to detect synchronous operations
17+
unsigned int before_pending = pending;
18+
call->callback(call->baton);
19+
20+
// If operation was synchronous (pending unchanged) and we're in exclusive mode,
21+
// reset locked and continue processing the queue.
22+
if (locked && pending == before_pending) {
23+
locked = false;
24+
continue;
25+
}
26+
```
27+
28+
**Files Changed**:
29+
- `src/database.cc`: Modified `Database::Process()` function
30+
- `test/serialization.test.js`: Added test cases for queue processing bug
31+
32+
**Affected Synchronous Operations**:
33+
- `SetLimit` - SQLite limit configuration
34+
- `SetBusyTimeout` - Busy timeout setting
35+
- `RegisterTraceCallback` - Trace callback registration
36+
- `RegisterProfileCallback` - Profile callback registration
37+
- `RegisterUpdateCallback` - Update hook registration
38+
- `Work_Wait` - Wait operation
39+
40+
**References**:
41+
- GitHub Issue: https://github.com/TryGhost/node-sqlite3/issues/1838
42+
43+
---
44+
545
### 2026-03-29: Security Hardening Implementation
646
747
**Decision**: Implement platform-specific security hardening flags in binding.gyp

memory-bank/progress.md

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,15 @@
11
# Progress Log
22

3-
## 2026-04-04:
3+
## 2026-04-10:
4+
5+
### fixed: queue processing deadlock in serialized mode
6+
- Fixed bug where operations get stuck in queue when using `db.serialize()` with synchronous operations
7+
- Issue: https://github.com/TryGhost/node-sqlite3/issues/1838
8+
- Modified `Database::Process()` in `src/database.cc` to detect synchronous operations
9+
- Added test cases in `test/serialization.test.js`
10+
- Solution: Track `pending` counter before/after callback to detect synchronous completion
11+
12+
## 2026-04-04:
413

514
### fixed: potential crash during shutdown
615
please see [microsoft/vscode-node-sqlite3/issues/67](https://github.com/microsoft/vscode-node-sqlite3/issues/67)

src/database.cc

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ Napi::FunctionReference Database::constructor;
1414
Napi::Object Database::Init(Napi::Env env, Napi::Object exports) {
1515
Napi::HandleScope scope(env);
1616
// declare napi_default_method here as it is only available in Node v14.12.0+
17-
auto napi_default_method = static_cast<napi_property_attributes>(napi_writable | napi_configurable);
17+
auto napi_default_method = static_cast<napi_property_attributes>(napi_writable | napi_configurable);
1818

1919
auto t = DefineClass(env, "Database", {
2020
InstanceMethod("close", &Database::Close, napi_default_method),
@@ -81,8 +81,21 @@ void Database::Process() {
8181
queue.pop();
8282
std::unique_ptr<Call> call(c);
8383
locked = call->exclusive;
84+
85+
// Track pending before callback to detect synchronous operations
86+
unsigned int before_pending = pending;
8487
call->callback(call->baton);
8588

89+
// If operation was synchronous (pending unchanged) and we're in exclusive mode,
90+
// reset locked and continue processing the queue.
91+
// Synchronous operations don't increment pending, so if pending is unchanged
92+
// and locked is true, the operation completed synchronously.
93+
if (locked && pending == before_pending) {
94+
locked = false;
95+
// Continue processing - don't break
96+
continue;
97+
}
98+
8699
if (locked) break;
87100
}
88101
}
@@ -340,7 +353,7 @@ Napi::Value Database::Configure(const Napi::CallbackInfo& info) {
340353
REQUIRE_ARGUMENTS(2);
341354

342355
Napi::Function handle;
343-
if (info[0].StrictEquals( Napi::String::New(env, "trace"))) {
356+
if (info[0].StrictEquals( Napi::String::New(env, "trace"))) {
344357
auto* baton = new Baton(db, handle);
345358
db->Schedule(RegisterTraceCallback, baton);
346359
}

test/serialization.test.js

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,3 +102,80 @@ describe('serialize(fn)', function() {
102102

103103
after(function(done) { db.close(done); });
104104
});
105+
106+
describe('serialize() queue processing with synchronous operations', function () {
107+
var db;
108+
109+
beforeEach(function (done) {
110+
db = new sqlite3.Database(':memory:', done);
111+
});
112+
113+
afterEach(function (done) {
114+
db.close(done);
115+
});
116+
117+
it('should process queued operations after configure in serialized mode', function (done) {
118+
// This test reproduces the bug where operations get stuck in the queue
119+
// when db.configure() is called during serialized mode with pending async work.
120+
// See: https://github.com/TryGhost/node-sqlite3/issues/1838
121+
122+
var LONG_QUERY = `WITH recursive recur(n)
123+
AS (SELECT 1
124+
UNION ALL
125+
SELECT n + 1
126+
FROM recur where n < 1000000
127+
)
128+
SELECT n FROM recur;`;
129+
130+
var executed = false;
131+
132+
db.serialize();
133+
134+
// Start a long-running async operation
135+
db.exec(LONG_QUERY);
136+
137+
// Queue a synchronous configure operation
138+
db.configure('limit', sqlite3.LIMIT_ATTACHED, 1);
139+
140+
// Queue another operation - this should execute after configure
141+
db.exec("SELECT 1", function (err) {
142+
if (err) return done(err);
143+
executed = true;
144+
});
145+
146+
// Give enough time for the bug to manifest
147+
setTimeout(function () {
148+
if (executed) {
149+
done();
150+
} else {
151+
done(new Error('Queue processing deadlock: SELECT 1 callback was never called'));
152+
}
153+
}, 2000);
154+
});
155+
156+
it('should process multiple configure calls in serialized mode', function (done) {
157+
var executed = false;
158+
159+
db.serialize();
160+
161+
db.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, value TEXT)");
162+
db.exec("INSERT INTO test VALUES (1, 'one')");
163+
164+
// Multiple configure calls should all be processed
165+
db.configure('limit', sqlite3.LIMIT_ATTACHED, 1);
166+
db.configure('busyTimeout', 1000);
167+
168+
db.exec("SELECT * FROM test", function (err, rows) {
169+
if (err) return done(err);
170+
executed = true;
171+
});
172+
173+
setTimeout(function () {
174+
if (executed) {
175+
done();
176+
} else {
177+
done(new Error('Queue processing deadlock: SELECT callback was never called'));
178+
}
179+
}, 1000);
180+
});
181+
});

0 commit comments

Comments
 (0)