Skip to content

Commit 5ac45f4

Browse files
surbhigarg92surbhigarg92
committed
gemini review comments fix
1 parent 76fda58 commit 5ac45f4

7 files changed

Lines changed: 105 additions & 21 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: 86 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ class CreatePoolWorker : public Napi::AsyncWorker {
6161

6262
Napi::Value CreatePoolWrapper(const Napi::CallbackInfo& info) {
6363
Napi::Env env = info.Env();
64+
if (info.Length() < 3) {
65+
Napi::Error::New(env, "CreatePoolWrapper requires 3 arguments").ThrowAsJavaScriptException();
66+
return env.Null();
67+
}
6468
std::string ua = info[0].As<Napi::String>();
6569
std::string cs = info[1].As<Napi::String>();
6670
Napi::Function cb = info[2].As<Napi::Function>();
@@ -79,6 +83,10 @@ class ClosePoolWorker : public Napi::AsyncWorker {
7983

8084
void Execute() override {
8185
result_ = ClosePool(poolId_);
86+
// Release the pinner ID of the response message to prevent native leak!
87+
if (result_.r0 > 0) {
88+
::Release(result_.r0);
89+
}
8290
}
8391

8492
void OnOK() override {
@@ -94,6 +102,10 @@ class ClosePoolWorker : public Napi::AsyncWorker {
94102

95103
Napi::Value ClosePoolWrapper(const Napi::CallbackInfo& info) {
96104
Napi::Env env = info.Env();
105+
if (info.Length() < 2) {
106+
Napi::Error::New(env, "ClosePoolWrapper requires 2 arguments").ThrowAsJavaScriptException();
107+
return env.Null();
108+
}
97109
int64_t pid = info[0].As<Napi::Number>().Int64Value();
98110
Napi::Function cb = info[1].As<Napi::Function>();
99111
ClosePoolWorker* worker = new ClosePoolWorker(cb, pid);
@@ -135,6 +147,10 @@ class CreateConnectionWorker : public Napi::AsyncWorker {
135147

136148
Napi::Value CreateConnectionWrapper(const Napi::CallbackInfo& info) {
137149
Napi::Env env = info.Env();
150+
if (info.Length() < 2) {
151+
Napi::Error::New(env, "CreateConnectionWrapper requires 2 arguments").ThrowAsJavaScriptException();
152+
return env.Null();
153+
}
138154
int64_t pid = info[0].As<Napi::Number>().Int64Value();
139155
Napi::Function cb = info[1].As<Napi::Function>();
140156
CreateConnectionWorker* worker = new CreateConnectionWorker(cb, pid);
@@ -152,6 +168,10 @@ class CloseConnectionWorker : public Napi::AsyncWorker {
152168

153169
void Execute() override {
154170
result_ = CloseConnection(poolId_, connId_);
171+
// Release the pinner ID of the response message to prevent native leak!
172+
if (result_.r0 > 0) {
173+
::Release(result_.r0);
174+
}
155175
}
156176

157177
void OnOK() override {
@@ -167,6 +187,10 @@ class CloseConnectionWorker : public Napi::AsyncWorker {
167187

168188
Napi::Value CloseConnectionWrapper(const Napi::CallbackInfo& info) {
169189
Napi::Env env = info.Env();
190+
if (info.Length() < 3) {
191+
Napi::Error::New(env, "CloseConnectionWrapper requires 3 arguments").ThrowAsJavaScriptException();
192+
return env.Null();
193+
}
170194
int64_t pid = info[0].As<Napi::Number>().Int64Value();
171195
int64_t cid = info[1].As<Napi::Number>().Int64Value();
172196
Napi::Function cb = info[2].As<Napi::Function>();
@@ -200,6 +224,10 @@ class ExecuteWorker : public Napi::AsyncWorker {
200224
} else {
201225
obj.Set("r4", env.Null());
202226
}
227+
// Release the pinner ID of the response buffer immediately after copy!
228+
if (result_.r0 > 0) {
229+
::Release(result_.r0);
230+
}
203231
Callback().Call({env.Null(), obj});
204232
}
205233
private:
@@ -210,6 +238,10 @@ class ExecuteWorker : public Napi::AsyncWorker {
210238

211239
Napi::Value ExecuteWrapper(const Napi::CallbackInfo& info) {
212240
Napi::Env env = info.Env();
241+
if (info.Length() < 4) {
242+
Napi::Error::New(env, "ExecuteWrapper requires 4 arguments").ThrowAsJavaScriptException();
243+
return env.Null();
244+
}
213245
int64_t pid = info[0].As<Napi::Number>().Int64Value();
214246
int64_t cid = info[1].As<Napi::Number>().Int64Value();
215247

@@ -246,6 +278,10 @@ class NextWorker : public Napi::AsyncWorker {
246278
} else {
247279
obj.Set("r4", env.Null());
248280
}
281+
// Release the pinner ID of the response buffer immediately after copy!
282+
if (result_.r0 > 0) {
283+
::Release(result_.r0);
284+
}
249285
Callback().Call({env.Null(), obj});
250286
}
251287
private:
@@ -292,15 +328,58 @@ class MetadataWorker : public Napi::AsyncWorker {
292328
} else {
293329
obj.Set("r4", env.Null());
294330
}
331+
// Release the pinner ID of the response buffer immediately after copy!
332+
if (result_.r0 > 0) {
333+
::Release(result_.r0);
334+
}
295335
Callback().Call({env.Null(), obj});
296336
}
297337
private:
298338
int64_t poolId_, connId_, rowsId_;
299339
Metadata_return result_;
300340
};
301341

342+
//
343+
// Worker 8: CloseRows asynchronously
344+
//
345+
class CloseRowsWorker : public Napi::AsyncWorker {
346+
public:
347+
CloseRowsWorker(Napi::Function& callback, int64_t poolId, int64_t connId, int64_t rowsId)
348+
: AsyncWorker(callback), poolId_(poolId), connId_(connId), rowsId_(rowsId), result_({0, 0, 0, 0, nullptr}) {}
349+
350+
void Execute() override {
351+
result_ = ::CloseRows(poolId_, connId_, rowsId_);
352+
}
353+
354+
void OnOK() override {
355+
Napi::Env env = Env();
356+
Napi::Object obj = Napi::Object::New(env);
357+
obj.Set("r0", Napi::Number::New(env, result_.r0));
358+
obj.Set("r1", Napi::Number::New(env, result_.r1));
359+
obj.Set("r2", Napi::Number::New(env, result_.r2));
360+
obj.Set("r3", Napi::Number::New(env, result_.r3));
361+
if (result_.r4 != nullptr && result_.r3 > 0) {
362+
obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
363+
} else {
364+
obj.Set("r4", env.Null());
365+
}
366+
// Release the pinner ID of the response message to prevent native leak!
367+
if (result_.r0 > 0) {
368+
::Release(result_.r0);
369+
}
370+
Callback().Call({env.Null(), obj});
371+
}
372+
private:
373+
int64_t poolId_, connId_, rowsId_;
374+
CloseRows_return result_;
375+
};
376+
302377
Napi::Value MetadataWrapper(const Napi::CallbackInfo& info) {
303378
Napi::Env env = info.Env();
379+
if (info.Length() < 4) {
380+
Napi::Error::New(env, "MetadataWrapper requires 4 arguments").ThrowAsJavaScriptException();
381+
return env.Null();
382+
}
304383
int64_t pid = info[0].As<Napi::Number>().Int64Value();
305384
int64_t cid = info[1].As<Napi::Number>().Int64Value();
306385
int64_t rid = info[2].As<Napi::Number>().Int64Value();
@@ -323,21 +402,17 @@ Napi::Value NativeRelease(const Napi::CallbackInfo& info) {
323402
// CloseRows dummy/missing implementation for POC length if needed, or we just rely on GC.
324403
Napi::Value CloseRowsWrapper(const Napi::CallbackInfo& info) {
325404
Napi::Env env = info.Env();
326-
if (info.Length() < 3) return env.Null();
405+
if (info.Length() < 4) {
406+
Napi::Error::New(env, "CloseRowsWrapper requires 4 arguments").ThrowAsJavaScriptException();
407+
return env.Null();
408+
}
327409
int64_t pid = info[0].As<Napi::Number>().Int64Value();
328410
int64_t cid = info[1].As<Napi::Number>().Int64Value();
329411
int64_t rid = info[2].As<Napi::Number>().Int64Value();
412+
Napi::Function cb = info[3].As<Napi::Function>();
330413

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-
}
414+
CloseRowsWorker* worker = new CloseRowsWorker(cb, pid, cid, rid);
415+
worker->Queue();
341416
return env.Undefined();
342417
}
343418

‎spannerlib/wrappers/spannerlib-node/src/ffi/utils.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ function invokeAsync(
4848
return reject(err);
4949
}
5050
if (result.r1 !== 0) {
51+
if (result.r0 && result.r0 > 0) {
52+
ffi.Release(result.r0);
53+
}
5154
if (result.r4 && result.r3 > 0) {
5255
const errorJson = result.r4.toString('utf8');
5356
try {

‎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
/**
@@ -50,11 +51,15 @@ export class Rows {
5051
async next(): Promise<unknown> {
5152
if (this.closed) throw new Error('Rows are already closed');
5253

54+
if (!this.connection.pool) {
55+
throw new Error('Connection must be bound to a Pool to fetch rows');
56+
}
57+
5358
const handled = await ffi.invokeAsync(
5459
'Next',
5560
null,
5661
null,
57-
this.connection.pool!.oid,
62+
this.connection.pool.oid,
5863
this.connection.oid,
5964
this.oid,
6065
1,

‎spannerlib/wrappers/spannerlib-node/test/rows.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ describe('Rows', () => {
4848
connection.pool = pool;
4949
connection.oid = 2;
5050

51-
const rows = new Rows(connection, 3);
51+
const rows = new Rows(connection, 3, 4);
5252

5353
// Create a dummy ListValue
5454
const listValue = ListValue.create({
@@ -88,7 +88,7 @@ describe('Rows', () => {
8888
connection.pool = pool;
8989
connection.oid = 2;
9090

91-
const rows = new Rows(connection, 3);
91+
const rows = new Rows(connection, 3, 4);
9292

9393
stub
9494
.onFirstCall()

0 commit comments

Comments
 (0)