Segfault when more Pool more connections requested than available as …

…'ready' and at least 1 is available
SAP · Jul 27, 2020 · 4640244 · 4640244
1 parent bcf23d1
commit 4640244
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 76 deletions.
diff --git a/CHANGES b/CHANGES
@@ -6,6 +6,7 @@ Change log
 
 - Environment object appended to error message when SAP NWRFC SDK not found or can't be loaded
 - Accidentally added p-queue dependency removed
+- Segfault when more Pool more connections requested than available as `ready` and at least 1 is available
 
 2.0.2 (2020-07-21)
 ------------------

diff --git a/package-lock.json b/package-lock.json
diff --git a/src/Pool.cc b/src/Pool.cc
@@ -121,12 +121,13 @@ namespace node_rfc
     public:
         AcquireAsync(Napi::Function &callback, const uint_t clients_requested, Pool *pool)
             : Napi::AsyncWorker(callback), clients_requested(clients_requested), pool(pool) {}
-        ~AcquireAsync() {}
+        ~AcquireAsync()
+        {
+        }
 
         void Execute()
         {
             pool->lockMutex();
-
             errorInfo.code = RFC_OK;
 
             uint_t ii = clients_requested;
@@ -136,8 +137,8 @@ namespace node_rfc
             {
                 if (it != pool->connReady.end())
                 {
-                    pool->connReady.erase(it);
-                    ready_connections.insert(*it++);
+                    ready_connections.insert(*it);
+                    pool->connReady.erase(it++);
                 }
                 else
                 {
@@ -239,7 +240,9 @@ namespace node_rfc
     public:
         ReleaseAsync(Napi::Function &callback, Pool *pool, std::set<Client *> clients)
             : Napi::AsyncWorker(callback), pool(pool), clients(clients) {}
-        ~ReleaseAsync() {}
+        ~ReleaseAsync()
+        {
+        }
 
         void Execute()
         {

diff --git a/test/pool/acquire-release.spec.js b/test/pool/acquire-release.spec.js
@@ -14,7 +14,7 @@
 
 "use strict";
 
-describe("Pool Acquire/Release/Ready", () => {
+describe("Pool Acquire/Release", () => {
     const setup = require("../utils/setup");
     const Pool = setup.Pool;
     const abapSystem = setup.abapSystem();
@@ -24,52 +24,61 @@ describe("Pool Acquire/Release/Ready", () => {
         //poolOptions: { low: 2, high: 4 },
     };
     let pool;
-    let acquired = [];
 
     beforeAll((done) => {
         pool = new Pool(poolConfiguration);
         done();
     });
 
     afterAll((done) => {
-        pool.clearAll();
         done();
     });
 
-    test("pool: acquire()", function () {
+    test("pool: acquire()", function (done) {
         expect.assertions(1);
-        return pool.acquire().then((client) => {
-            acquired.push(client);
+        pool.acquire().then((client) => {
             expect(client.alive).toBe(true);
+            done();
         });
     });
 
-    test("pool: acquire(multiple)", function () {
-        expect.assertions(4);
-        return pool.acquire(3).then((clients) => {
-            acquired.push(...clients);
-            expect(clients.length).toBe(3);
+    test("pool: acquire(multiple)", function (done) {
+        const N = 3;
+        expect.assertions(N + 1);
+        pool.acquire(N).then((clients) => {
+            expect(clients.length).toBe(N);
             clients.forEach((c) => {
                 expect(c.alive).toBe(true);
             });
+            done();
         });
     });
 
-    test("pool: release(single)", function () {
-        expect.assertions(1);
-        const client = acquired.pop();
-        const LEASED = pool.status.leased;
-        return pool.release(client).then(() => {
-            expect(acquired.length).toBe(LEASED - 1);
+    test("pool: release(single)", function (done) {
+        expect.assertions(3);
+        pool.acquire((err, client) => {
+            expect(err).not.toBeDefined();
+            const LEASED = pool.status.leased;
+            pool.release(client, (err) => {
+                expect(err).not.toBeDefined();
+                expect(pool.status.leased).toBe(LEASED - 1);
+                done();
+            });
         });
     });
 
-    test("pool: release(multiple)", function () {
-        expect.assertions(1);
-        const LEASED = pool.status.leased;
-        const return_clients = acquired.slice(0, 2);
-        return pool.release(return_clients).then(() => {
-            expect(pool.status.leased).toBe(LEASED - 2);
+    test("pool: release(multiple)", function (done) {
+        expect.assertions(4);
+        const N = 3;
+        pool.acquire(N, (err, clients) => {
+            expect(err).not.toBeDefined();
+            expect(clients.length).toBe(N);
+            const LEASED = pool.status.leased;
+            pool.release(clients, (err) => {
+                expect(err).not.toBeDefined();
+                expect(pool.status.leased).toBe(LEASED - N);
+                done();
+            });
         });
     });
 

diff --git a/test/pool/ready.spec.js b/test/pool/ready.spec.js
@@ -34,7 +34,6 @@ describe("Pool Acquire/Release/Ready", () => {
     });
 
     afterAll((done) => {
-        pool.clearAll();
         done();
     });