Skip to content

Commit 04c5448

Browse files
CMCDragonkaiCMCDragonkai
committed
Fix database->isClosing_ and Transaction::DecrementPendingWork should be checking for pendingWork_ == 1 
1 parent c909c02 commit 04c5448

File tree

4 files changed

+138
-57
lines changed

4 files changed

+138
-57
lines changed

‎src/rocksdb/napi/index.cpp

Lines changed: 51 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -86,28 +86,37 @@ static void env_cleanup_hook(void* arg) {
8686
* - `TransactionRollbackDo`
8787
*/
8888
static void IteratorCloseDo(napi_env env, Iterator* iterator, napi_value cb) {
89+
LOG_DEBUG("IteratorCloseDo:Calling IteratorCloseDo\n");
8990
CloseIteratorWorker* worker = new CloseIteratorWorker(env, iterator, cb);
9091
iterator->isClosing_ = true;
9192
// The only pending work for iterator is the `NextWorker`
9293
if (!iterator->nexting_) {
94+
LOG_DEBUG("IteratorCloseDo:Queued CloseIteratorWorker\n");
9395
worker->Queue(env);
96+
LOG_DEBUG("IteratorCloseDo:Called IteratorCloseDo\n");
9497
return;
9598
}
99+
LOG_DEBUG("IteratorCloseDo:Delayed CloseIteratorWorker\n");
96100
iterator->closeWorker_ = worker;
101+
LOG_DEBUG("IteratorCloseDo:Called IteratorCloseDo\n");
97102
}
98103

99104
/**
100105
* Used by `transactionRollback` and `dbClose`
101106
*/
102107
static void TransactionRollbackDo(napi_env env, Transaction* transaction,
103108
napi_value cb) {
109+
LOG_DEBUG("TransactionRollbackDo:Calling TransactionRollBackDo\n");
104110
TransactionRollbackWorker* worker =
105111
new TransactionRollbackWorker(env, transaction, cb);
106112
transaction->isRollbacking_ = true;
107113
if (!transaction->HasPendingWork()) {
114+
LOG_DEBUG("TransactionRollbackDo:Queued TransactionRollbackWorker\n");
108115
worker->Queue(env);
116+
LOG_DEBUG("TransactionRollbackDo:Called TransactionRollBackDo\n");
109117
return;
110118
}
119+
LOG_DEBUG("TransactionRollbackDo:Delayed TransactionRollbackWorker\n");
111120
transaction->closeWorker_ = worker;
112121
napi_value noop;
113122
napi_create_function(env, NULL, 0, noop_callback, NULL, &noop);
@@ -119,17 +128,22 @@ static void TransactionRollbackDo(napi_env env, Transaction* transaction,
119128
if (iterator->isClosing_ || iterator->hasClosed_) {
120129
continue;
121130
}
131+
LOG_DEBUG("TransactionRollbackDo:Closing Iterator %d\n", iterator->id_);
122132
IteratorCloseDo(env, iterator, noop);
123133
}
134+
LOG_DEBUG("TransactionRollbackDo:Called TransactionRollBackDo\n");
124135
}
125136

126137
/**
127138
* Used by `snapshotRelease` and `dbClose`
128139
*/
129140
static void SnapshotReleaseDo(napi_env env, Snapshot* snapshot, napi_value cb) {
141+
LOG_DEBUG("SnapshotReleaseDo:Calling SnapshotReleaseDo\n");
130142
SnapshotReleaseWorker* worker = new SnapshotReleaseWorker(env, snapshot, cb);
131143
snapshot->isReleasing_ = true;
132144
worker->Queue(env);
145+
LOG_DEBUG("SnapshotReleaseDo:Queued SnapshotReleaseWorker\n");
146+
LOG_DEBUG("SnapshotReleaseDo:Called SnapshotReleaseDo\n");
133147
}
134148

135149
/**
@@ -138,15 +152,17 @@ static void SnapshotReleaseDo(napi_env env, Snapshot* snapshot, napi_value cb) {
138152
* with no references and no concurrent workers
139153
*/
140154
static void GCDatabase(napi_env env, void* data, void* hint) {
141-
LOG_DEBUG("Garbage Collecting Database\n");
155+
LOG_DEBUG("GCDatabase:Garbage Collecting Database\n");
142156
if (data != nullptr) {
143157
auto database = static_cast<Database*>(data);
144158
napi_remove_env_cleanup_hook(env, env_cleanup_hook, database);
145-
database->Close();
146-
database->Detach(env);
159+
if (!database->isClosing_ && !database->hasClosed_) {
160+
database->Close();
161+
database->Detach(env);
162+
}
147163
delete database;
148164
}
149-
LOG_DEBUG("Garbage Collected Database\n");
165+
LOG_DEBUG("GCDatabase:Garbage Collected Database\n");
150166
}
151167

152168
/**
@@ -155,12 +171,12 @@ static void GCDatabase(napi_env env, void* data, void* hint) {
155171
* with no references and no concurrent workers
156172
*/
157173
static void GCBatch(napi_env env, void* data, void* hint) {
158-
LOG_DEBUG("Garbage Collecting Batch\n");
174+
LOG_DEBUG("GCBatch:Garbage Collecting Batch\n");
159175
if (data) {
160176
auto batch = static_cast<Batch*>(data);
161177
delete batch;
162178
}
163-
LOG_DEBUG("Garbage Collected Batch\n");
179+
LOG_DEBUG("GCBatch:Garbage Collected Batch\n");
164180
}
165181

166182
/**
@@ -169,14 +185,16 @@ static void GCBatch(napi_env env, void* data, void* hint) {
169185
* with no references and no concurrent workers
170186
*/
171187
static void GCIterator(napi_env env, void* data, void* hint) {
172-
LOG_DEBUG("Garbage Collecting Iterator\n");
188+
LOG_DEBUG("GCIterator:Garbage collecting Iterator\n");
173189
if (data != nullptr) {
174190
auto iterator = static_cast<Iterator*>(data);
175-
iterator->Close();
176-
iterator->Detach(env);
191+
if (!iterator->isClosing_ && !iterator->hasClosed_) {
192+
iterator->Close();
193+
iterator->Detach(env);
194+
}
177195
delete iterator;
178196
}
179-
LOG_DEBUG("Garbage Collected Iterator\n");
197+
LOG_DEBUG("GCIterator:Garbage Collected Iterator\n");
180198
}
181199

182200
/**
@@ -185,16 +203,17 @@ static void GCIterator(napi_env env, void* data, void* hint) {
185203
* with no references and no concurrent workers
186204
*/
187205
static void GCTransaction(napi_env env, void* data, void* hint) {
188-
LOG_DEBUG("Garbage Collecting Transaction\n");
206+
LOG_DEBUG("GCTransaction:Garbage Collecting Transaction\n");
189207
if (data != nullptr) {
190208
auto transaction = static_cast<Transaction*>(data);
191-
if (!transaction->hasCommitted_) {
209+
if (!transaction->isCommitting_ && !transaction->hasCommitted_ &&
210+
!transaction->isRollbacking_ && !transaction->hasRollbacked_) {
192211
transaction->Rollback();
212+
transaction->Detach(env);
193213
}
194-
transaction->Detach(env);
195214
delete transaction;
196215
}
197-
LOG_DEBUG("Garbage Collected Transaction\n");
216+
LOG_DEBUG("GCTransaction:Garbage Collected Transaction\n");
198217
}
199218

200219
/**
@@ -203,14 +222,16 @@ static void GCTransaction(napi_env env, void* data, void* hint) {
203222
* with no references and no concurrent workers
204223
*/
205224
static void GCSnapshot(napi_env env, void* data, void* hint) {
206-
LOG_DEBUG("Garbage Collecting Snapshot\n");
225+
LOG_DEBUG("GCSnapshot:Garbage Collecting Snapshot\n");
207226
if (data != nullptr) {
208227
auto snapshot = static_cast<Snapshot*>(data);
209-
snapshot->Release();
210-
snapshot->Detach(env);
228+
if (!snapshot->isReleasing_ && !snapshot->hasReleased_) {
229+
snapshot->Release();
230+
snapshot->Detach(env);
231+
}
211232
delete snapshot;
212233
}
213-
LOG_DEBUG("Garbage Collected Snapshot\n");
234+
LOG_DEBUG("GCSnapshot:Garbage Collected Snapshot\n");
214235
}
215236

216237
/**
@@ -219,12 +240,12 @@ static void GCSnapshot(napi_env env, void* data, void* hint) {
219240
* with no references and no concurrent workers
220241
*/
221242
static void GCTransactionSnapshot(napi_env env, void* data, void* hint) {
222-
LOG_DEBUG("Garbage Collecting TransactionSnapshot\n");
243+
LOG_DEBUG("GCTransactionSnapshot:Garbage Collecting TransactionSnapshot\n");
223244
if (data) {
224245
auto snapshot = static_cast<TransactionSnapshot*>(data);
225246
delete snapshot;
226247
}
227-
LOG_DEBUG("Garbage Collected TransactionSnapshot\n");
248+
LOG_DEBUG("GCTransactionSnapshot:Garbage Collected TransactionSnapshot\n");
228249
}
229250

230251
/**
@@ -314,14 +335,19 @@ NAPI_METHOD(dbOpen) {
314335
* This is asynchronous
315336
*/
316337
NAPI_METHOD(dbClose) {
338+
LOG_DEBUG("dbClose:Calling dbClose\n");
317339
NAPI_ARGV(2);
318340
NAPI_DB_CONTEXT();
319341
napi_value callback = argv[1];
320342
CloseWorker* worker = new CloseWorker(env, database, callback);
343+
database->isClosing_ = true;
321344
if (!database->HasPendingWork()) {
345+
LOG_DEBUG("dbClose:Queued CloseWorker\n");
322346
worker->Queue(env);
347+
LOG_DEBUG("dbClose:Called dbClose\n");
323348
NAPI_RETURN_UNDEFINED();
324349
}
350+
LOG_DEBUG("dbClose:Delayed CloseWorker\n");
325351
database->closeWorker_ = worker;
326352
napi_value noop;
327353
napi_create_function(env, NULL, 0, noop_callback, NULL, &noop);
@@ -333,6 +359,7 @@ NAPI_METHOD(dbClose) {
333359
if (iterator->isClosing_ || iterator->hasClosed_) {
334360
continue;
335361
}
362+
LOG_DEBUG("dbClose:Closing Iterator %d\n", iterator->id_);
336363
IteratorCloseDo(env, iterator, noop);
337364
}
338365
std::map<uint32_t, Transaction*> transactions = database->transactions_;
@@ -344,6 +371,7 @@ NAPI_METHOD(dbClose) {
344371
transaction->isRollbacking_ || transaction->hasRollbacked_) {
345372
continue;
346373
}
374+
LOG_DEBUG("dbClose:Rollbacking Transaction %d\n", transaction->id_);
347375
TransactionRollbackDo(env, transaction, noop);
348376
}
349377
std::map<uint32_t, Snapshot*> snapshots = database->snapshots_;
@@ -354,8 +382,11 @@ NAPI_METHOD(dbClose) {
354382
if (snapshot->isReleasing_ || snapshot->hasReleased_) {
355383
continue;
356384
}
385+
LOG_DEBUG("dbClose:Releasing Snapshot %d\n", snapshot->id_);
357386
SnapshotReleaseDo(env, snapshot, noop);
358387
}
388+
389+
LOG_DEBUG("dbClose:Called dbClose\n");
359390
NAPI_RETURN_UNDEFINED();
360391
}
361392

‎src/rocksdb/napi/iterator.cpp

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -277,8 +277,8 @@ Iterator::Iterator(Database* database, const uint32_t id, const bool reverse,
277277
isClosing_(false),
278278
closeWorker_(nullptr),
279279
ref_(nullptr) {
280-
LOG_DEBUG("Iterator:Constructing Iterator %d from Database\n", id_);
281-
LOG_DEBUG("Iterator:Constructed Iterator %d from Database\n", id_);
280+
LOG_DEBUG("Iterator %d:Constructing from Database\n", id_);
281+
LOG_DEBUG("Iterator %d:Constructed from Database\n", id_);
282282
}
283283

284284
Iterator::Iterator(Transaction* transaction, const uint32_t id,
@@ -301,45 +301,55 @@ Iterator::Iterator(Transaction* transaction, const uint32_t id,
301301
isClosing_(false),
302302
closeWorker_(nullptr),
303303
ref_(nullptr) {
304-
LOG_DEBUG("Iterator:Constructing Iterator %d from Transaction %d\n", id_,
304+
LOG_DEBUG("Iterator %d:Constructing from Transaction %d\n", id_,
305305
transaction->id_);
306-
LOG_DEBUG("Iterator:Constructed Iterator %d from Transaction %d\n", id_,
306+
LOG_DEBUG("Iterator %d:Constructed from Transaction %d\n", id_,
307307
transaction->id_);
308308
}
309309

310310
Iterator::~Iterator() {
311-
LOG_DEBUG("Iterator:Destroying Iterator %d\n", id_);
311+
LOG_DEBUG("Iterator %d:Destroying\n", id_);
312312
BaseIterator::~BaseIterator();
313-
LOG_DEBUG("Iterator:Destroyed Iterator %d\n", id_);
313+
LOG_DEBUG("Iterator %d:Destroyed\n", id_);
314314
};
315315

316316
void Iterator::Attach(napi_env env, napi_value iterator_ref) {
317+
LOG_DEBUG("Iterator %d:Calling Attach\n", id_);
317318
assert(database_ != nullptr || transaction_ != nullptr);
318-
if (ref_ != nullptr) return;
319+
if (ref_ != nullptr) {
320+
LOG_DEBUG("Iterator %d:Called Attach\n", id_);
321+
return;
322+
}
319323
NAPI_STATUS_THROWS_VOID(napi_create_reference(env, iterator_ref, 1, &ref_));
320324
if (database_ != nullptr) {
321325
database_->AttachIterator(env, id_, this);
322326
} else if (transaction_ != nullptr) {
323327
transaction_->AttachIterator(env, id_, this);
324328
}
329+
LOG_DEBUG("Iterator %d:Called Attach\n", id_);
325330
}
326331

327332
void Iterator::Detach(napi_env env) {
333+
LOG_DEBUG("Iterator %d:Calling Detach\n", id_);
328334
assert(database_ != nullptr || transaction_ != nullptr);
329-
if (ref_ == nullptr) return;
335+
if (ref_ == nullptr) {
336+
LOG_DEBUG("Iterator %d:Called Detach\n", id_);
337+
return;
338+
}
330339
if (database_ != nullptr) {
331340
database_->DetachIterator(env, id_);
332341
} else if (transaction_ != nullptr) {
333342
transaction_->DetachIterator(env, id_);
334343
}
335344
NAPI_STATUS_THROWS_VOID(napi_delete_reference(env, ref_));
336345
ref_ = nullptr;
346+
LOG_DEBUG("Iterator %d:Called Detach\n", id_);
337347
}
338348

339349
void Iterator::Close() {
340-
LOG_DEBUG("Iterator:Closing Iterator %d\n", id_);
350+
LOG_DEBUG("Iterator %d:Calling Close\n", id_);
341351
BaseIterator::Close();
342-
LOG_DEBUG("Iterator:Closed Iterator %d\n", id_);
352+
LOG_DEBUG("Iterator %d:Called Close\n", id_);
343353
}
344354

345355
bool Iterator::ReadMany(uint32_t size) {

‎src/rocksdb/napi/snapshot.cpp

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,54 +19,73 @@ Snapshot::Snapshot(Database* database, const uint32_t id)
1919
isReleasing_(false),
2020
hasReleased_(false),
2121
ref_(NULL) {
22-
LOG_DEBUG("Snapshot:Constructing Snapshot %d\n", id_);
22+
LOG_DEBUG("Snapshot %d:Constructing Snapshot from Database\n", id_);
2323
snap_ = database->NewSnapshot();
24-
LOG_DEBUG("Snapshot:Constructed Snapshot %d\n", id_);
24+
LOG_DEBUG("Snapshot %d:Constructed Snapshot from Database\n", id_);
2525
}
2626

2727
Snapshot::~Snapshot() {
28-
LOG_DEBUG("Snapshot:Destroying Snapshot %d\n", id_);
28+
LOG_DEBUG("Snapshot %d:Destroying\n", id_);
2929
assert(hasReleased_);
3030
// Cannot delete `snap_` because it is already deleted by `ReleaseSnapshot`
31-
LOG_DEBUG("Snapshot:Destroyed Snapshot %d\n", id_);
31+
LOG_DEBUG("Snapshot %d:Destroyed\n", id_);
3232
}
3333

3434
void Snapshot::Attach(napi_env env, napi_value snapshot_ref) {
35-
if (ref_ != nullptr) return;
35+
LOG_DEBUG("Snapshot %d:Calling Attach\n", id_);
36+
if (ref_ != nullptr) {
37+
LOG_DEBUG("Snapshot %d:Called Attach\n", id_);
38+
return;
39+
}
3640
NAPI_STATUS_THROWS_VOID(napi_create_reference(env, snapshot_ref, 1, &ref_));
3741
database_->AttachSnapshot(env, id_, this);
42+
LOG_DEBUG("Snapshot %d:Called Attach\n", id_);
3843
}
3944

4045
void Snapshot::Detach(napi_env env) {
41-
if (ref_ == nullptr) return;
46+
LOG_DEBUG("Snapshot %d:Calling Detach\n", id_);
47+
if (ref_ == nullptr) {
48+
LOG_DEBUG("Snapshot %d:Called Detach\n", id_);
49+
return;
50+
}
4251
database_->DetachSnapshot(env, id_);
4352
NAPI_STATUS_THROWS_VOID(napi_delete_reference(env, ref_));
4453
ref_ = nullptr;
54+
LOG_DEBUG("Snapshot %d:Called Detach\n", id_);
4555
}
4656

4757
void Snapshot::Release() {
48-
LOG_DEBUG("Snapshot:Releasing Snapshot %d\n", id_);
49-
if (hasReleased_) return;
58+
LOG_DEBUG("Snapshot %d:Calling Release\n", id_);
59+
if (hasReleased_) {
60+
LOG_DEBUG("Snapshot %d:Called Release\n", id_);
61+
return;
62+
}
5063
hasReleased_ = true;
5164
// This deletes also deletes `rocksdb::Snapshot`
5265
database_->ReleaseSnapshot(snap_);
53-
LOG_DEBUG("Snapshot:Released Snapshot %d\n", id_);
66+
LOG_DEBUG("Snapshot %d:Called Release\n", id_);
5467
}
5568

5669
const rocksdb::Snapshot* Snapshot::snapshot() const { return snap_; }
5770

5871
TransactionSnapshot::TransactionSnapshot(Transaction* transaction) {
59-
LOG_DEBUG("TransactionSnapshot:Constructing Snapshot from Transaction\n");
72+
LOG_DEBUG(
73+
"TransactionSnapshot:Constructing TransactionSnapshot from Transaction "
74+
"%d\n",
75+
transaction->id_);
6076
// This ensures that the transaction has consistent writes
6177
transaction->SetSnapshot();
6278
// Use this snapshot to get consistent reads
6379
snap_ = transaction->GetSnapshot();
64-
LOG_DEBUG("TransactionSnapshot:Constructed Snapshot from Transaction\n");
80+
LOG_DEBUG(
81+
"TransactionSnapshot:Constructed TransactionSnapshot from Transaction "
82+
"%d\n",
83+
transaction->id_);
6584
}
6685

6786
TransactionSnapshot::~TransactionSnapshot() {
68-
LOG_DEBUG("TransactionSnapshot:Destroying Snapshot from Transaction\n");
69-
LOG_DEBUG("TransactionSnapshot:Destoryed Snapshot from Transaction\n");
87+
LOG_DEBUG("TransactionSnapshot:Destroying\n");
88+
LOG_DEBUG("TransactionSnapshot:Destroyed\n");
7089
}
7190

7291
const rocksdb::Snapshot* TransactionSnapshot::snapshot() const { return snap_; }

0 commit comments

Comments
 (0)