Skip to content

Commit d0fd19c

Browse files
committed
gemini review comments fix
1 parent 76fda58 commit d0fd19c

5 files changed

Lines changed: 53 additions & 19 deletions

File tree

spannerlib/wrappers/spannerlib-node/scripts/fix-extensions.cjs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,8 @@ function traverseDir(dir) {
2424
traverseDir(fullPath);
2525
} else if (fullPath.endsWith('.cjs')) {
2626
let content = fs.readFileSync(fullPath, 'utf8');
27-
// Replace require('./... .js') with require('./... .cjs')
28-
content = content.replace(/require\(['"](\.\/[^'"]+)\.js['"]\)/g, "require('$1.cjs')");
29-
// Also handle ../
30-
content = content.replace(/require\(['"](\.\.\/[^'"]+)\.js['"]\)/g, "require('$1.cjs')");
27+
// Replace require paths starting with . / or .. / and ending with .js
28+
content = content.replace(/require\(['"]((\.|\.\.)\/[^'"]+)\.js['"]\)/g, "require('$1.cjs')");
3129
// Fix import.meta.url syntax error in CommonJS
3230
content = content.replace(/import\.meta\.url/g, '""');
3331
fs.writeFileSync(fullPath, content);

spannerlib/wrappers/spannerlib-node/src/cpp/addon.cc

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,41 @@ class MetadataWorker : public Napi::AsyncWorker {
299299
Metadata_return result_;
300300
};
301301

302+
//
303+
// Worker 8: CloseRows asynchronously
304+
//
305+
class CloseRowsWorker : public Napi::AsyncWorker {
306+
public:
307+
CloseRowsWorker(Napi::Function& callback, int64_t poolId, int64_t connId, int64_t rowsId)
308+
: AsyncWorker(callback), poolId_(poolId), connId_(connId), rowsId_(rowsId), result_({0, 0, 0, 0, nullptr}) {}
309+
310+
void Execute() override {
311+
result_ = ::CloseRows(poolId_, connId_, rowsId_);
312+
// Release the pinner ID of the response message to prevent native leak!
313+
if (result_.r0 > 0) {
314+
::Release(result_.r0);
315+
}
316+
}
317+
318+
void OnOK() override {
319+
Napi::Env env = Env();
320+
Napi::Object obj = Napi::Object::New(env);
321+
obj.Set("r0", Napi::Number::New(env, result_.r0));
322+
obj.Set("r1", Napi::Number::New(env, result_.r1));
323+
obj.Set("r2", Napi::Number::New(env, result_.r2));
324+
obj.Set("r3", Napi::Number::New(env, result_.r3));
325+
if (result_.r4 != nullptr && result_.r3 > 0) {
326+
obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
327+
} else {
328+
obj.Set("r4", env.Null());
329+
}
330+
Callback().Call({env.Null(), obj});
331+
}
332+
private:
333+
int64_t poolId_, connId_, rowsId_;
334+
CloseRows_return result_;
335+
};
336+
302337
Napi::Value MetadataWrapper(const Napi::CallbackInfo& info) {
303338
Napi::Env env = info.Env();
304339
int64_t pid = info[0].As<Napi::Number>().Int64Value();
@@ -323,21 +358,14 @@ Napi::Value NativeRelease(const Napi::CallbackInfo& info) {
323358
// CloseRows dummy/missing implementation for POC length if needed, or we just rely on GC.
324359
Napi::Value CloseRowsWrapper(const Napi::CallbackInfo& info) {
325360
Napi::Env env = info.Env();
326-
if (info.Length() < 3) return env.Null();
361+
if (info.Length() < 4) return env.Null();
327362
int64_t pid = info[0].As<Napi::Number>().Int64Value();
328363
int64_t cid = info[1].As<Napi::Number>().Int64Value();
329364
int64_t rid = info[2].As<Napi::Number>().Int64Value();
365+
Napi::Function cb = info[3].As<Napi::Function>();
330366

331-
// N-API sync close implementation
332-
CloseRows(pid, cid, rid);
333-
334-
// invokeAsync appends a callback at the end of properties
335-
if (info.Length() >= 4 && info[3].IsFunction()) {
336-
Napi::Object obj = Napi::Object::New(env);
337-
obj.Set("r1", Napi::Number::New(env, 0));
338-
Napi::Function cb = info[3].As<Napi::Function>();
339-
cb.Call({env.Null(), obj}); // Mock empty GoReturnTuple callback
340-
}
367+
CloseRowsWorker* worker = new CloseRowsWorker(cb, pid, cid, rid);
368+
worker->Queue();
341369
return env.Undefined();
342370
}
343371

spannerlib/wrappers/spannerlib-node/src/lib/connection.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export class Connection {
5454

5555
c.oid = handled.objectId;
5656
c.pinnerId = handled.pinnerId;
57+
spannerLib.register(c, handled.pinnerId);
5758
return c;
5859
}
5960

@@ -89,8 +90,9 @@ export class Connection {
8990
serializedPb
9091
);
9192
const rowsId = rowsResult.objectId;
93+
const rowsPinnerId = rowsResult.pinnerId;
9294

93-
return new Rows(this, rowsId);
95+
return new Rows(this, rowsId, rowsPinnerId);
9496
}
9597

9698
/**

spannerlib/wrappers/spannerlib-node/src/lib/pool.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ export class Pool {
5252

5353
p.oid = handled.objectId;
5454
p.pinnerId = handled.pinnerId;
55+
spannerLib.register(p, handled.pinnerId);
5556
return p;
5657
}
5758

spannerlib/wrappers/spannerlib-node/src/lib/rows.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,12 @@ export class Rows {
3333
public oid: number;
3434
public pinnerId: number | null;
3535
public closed: boolean;
36-
constructor(connection: Connection, oid: number) {
36+
constructor(connection: Connection, oid: number, pinnerId: number) {
3737
this.connection = connection;
3838
this.oid = oid;
39-
this.pinnerId = null;
39+
this.pinnerId = pinnerId;
4040
this.closed = false;
41+
spannerLib.register(this, pinnerId);
4142
}
4243

4344
/**
@@ -65,7 +66,11 @@ export class Rows {
6566
return null;
6667
}
6768

68-
return ListValue.decode(handled.protobufBytes);
69+
const row = ListValue.decode(handled.protobufBytes);
70+
if (handled.pinnerId && handled.pinnerId > 0) {
71+
ffi.Release(handled.pinnerId);
72+
}
73+
return row;
6974
}
7075

7176
/**

0 commit comments

Comments
 (0)