SAP / node-rfc

Asynchronous, non-blocking SAP NW RFC SDK bindings for Node.js
Apache License 2.0
249 stars 73 forks source link

Connection count limited to 4 #232

Closed MrSJS closed 2 years ago

MrSJS commented 2 years ago

I only get 4 concurrent Connections to SAP. Is there a setting for this? If I make an rfc call five times, the fifth has to wait until one of the four has finished. Therefore, the fifth needs a minute longer.

    var newPool = require('node-rfc').Pool;
    console.log("new");
    var pool = new newPool({ 
        connectionParameters: {
            user: "RFC_USER",  //your credentials
            passwd: "passwd",
            ashost: "sap.ppp.local",
            sysnr: "00",
            client: "100",
            lang: "EN"
        }
        , clientOptions: {stateless:true}
    });

var connect = function(){
    pool.acquire().then(client => {

        var date = new Date();
        var start = ("0" + date.getHours() ).slice(-2) + ":" + ("0" + date.getMinutes()).slice(-2) + ":" + ("0" + date.getSeconds()).slice(-2);

        console.log("start ping: " + start);

        //(only 4 calls parallel?)
        client.call("SPTA_RFC_PING_AND_WAIT", {"SECONDS": 60}).then(res => {

            pool.release(client);

            console.log(res);

            var date = new Date();
            var end = ("0" + date.getHours() ).slice(-2) + ":" + ("0" + date.getMinutes()).slice(-2) + ":" + ("0" + date.getSeconds()).slice(-2);

            console.log("from: " + start + " end: " + end);

        }).catch(err => {
            console.error("[sapRFC:call] ", err);
            pool.release(client);
        });

    }).catch(err => {
        console.error("[sapRFC:pool.aquire] ", err);
    });
};

// 5 requests
connect();
connect();
connect();
connect();
connect();

image

bsrdjan commented 2 years ago

Yes, 4 connections are the default for the Pool but can be changed using Pool options:

const pool = new Pool({
    connectionParameters: connParams,
    poolOptions: { high: 14 }
});

Documentation: SAP/node-rfc/doc/usage.md#connection-pool

Max number of concurrent connections allowed per user is also restricted in ABAP system settings. A corresponding error will be raised if that limitation exceeded.

MrSJS commented 2 years ago

Thank you for your quick response. But the poolOptions do not affect this.

image

Could it be a different library like this one in this issue? https://github.com/jdorner/node-sapnwrfc/issues/28

And with the same RFC user and the SAP Java Connector 3.1 SDK there is no limit.

MrSJS commented 2 years ago

before node.js I used the Wildfly Server with SAP Java Connector 3.1 SDK with the same RFC user. I therefore strongly assume that it is not due to the settings in SAP.

MrSJS commented 2 years ago

and i would be very grateful if you could try my code snippet from above in your system to see if you get the same output. From our clients we sometimes have larger requests to SAP and we would lose a lot of performance if only 4 parallel connections were possible.

MrSJS commented 2 years ago

I have to run node like this in cmd SET UV_THREADPOOL_SIZE=6 && node myFile.js

Default value is 4

bsrdjan commented 2 years ago

Thank you @MrSJS for this good catch, I just started testing but you was faster :)

This libuv threadpool default restriction is documented but I never used it or came across it: http://docs.libuv.org/en/v1.x/threadpool.html. It shall be added to node-rfc README/documentation and can be classified as a documentation bug.

It works exactly the same way in my test environment, using minor modified script, to easier count threads and test with different UV_THREADPOOL_SIZE values.

const addon = require("../../lib");
console.log("new");

const getTime = function () {
    const date = new Date();
    return (
        ("0" + date.getHours()).slice(-2) +
        ":" +
        ("0" + date.getMinutes()).slice(-2) +
        ":" +
        ("0" + date.getSeconds()).slice(-2)
    );
};

const pool = new addon.Pool({
    connectionParameters: { dest: "MME" },
});

const CountUp = 10;
let countDown = 0;

const connect = function (id) {
    pool.acquire()
        .then((client) => {
            const start = getTime();
            console.log(id, "start ping: " + start);

            //(only 4 calls parallel?)
            client
                .call("SPTA_RFC_PING_AND_WAIT", { SECONDS: 5 })
                .then((res) => {
                    pool.release(client);

                    //console.log(res);

                    const end = getTime();

                    console.log(countDown++, "from: " + start + " end: " + end);
                })
                .catch((err) => {
                    console.error("[sapRFC:call] ", err);
                    pool.release(client);
                });
        })
        .catch((err) => {
            console.error("[sapRFC:pool.aquire] ", err);
        });
};

// 10 requests
for (let id = 0; id < CountUp; id++) {
    connect(id);
}
MrSJS commented 2 years ago

Thanks you very much. But the question is, why do we have to increase the number of threads here in contrast to other asynchronous functions? We need a new thread for each parallel RFC call. Some articles write that we should only use as many threads in node as we have logical cores in our hardware ...

https://dev.to/johnjardincodes/increase-node-js-performance-with-libuv-thread-pool-5h10

MrSJS commented 2 years ago

By default, node can process a lot more http requests in parallel while waiting for a response, for example

MrSJS commented 2 years ago

It seems like Node.js can't use its Asynchronous system APIs for node-rfc. Asynchronous system APIs are used by Node.js whenever possible, but where they do not exist, libuv's threadpool is used to create asynchronous node APIs based on synchronous system APIs. And libuv is not thread safe.

https://nodejs.org/api/cli.html#cli_uv_threadpool_size_size

bsrdjan commented 2 years ago

It seems like Node.js can't use its Asynchronous system APIs for node-rfc. Asynchronous system APIs are used by Node.js whenever possible, but where they do not exist, libuv's threadpool is used to create asynchronous node APIs based on synchronous system APIs. And libuv is not thread safe.

Correct, the Node.js Worker Pool async API is using libuv to offload/displace operations of C++ add-ons (like node-rfc) from the event loop:

https://nodejs.org/en/docs/guides/dont-block-the-event-loop/

Other async API could start and notify on RFC call completion but would also block the main loop. The current libuv maximum thread pool size is 1024. Would it be enough for your scenario?

MrSJS commented 2 years ago

Thank you very much. Of course, 1024 would be enough for my scenario. I think it would be ideal if node-rfc could avoid the fallback to libuv. So is there no way that node-rfc can use the asynchronous system APIs from Node.js for this project?

bsrdjan commented 2 years ago

I don't see the possibility to use something else instead of libuv but we can also ask the Node.js team. libuv is listed as Node.js standard dependency and I am not aware of alternatives.

Which problem would be solved by not using libuv?

MrSJS commented 2 years ago

In relation to the link you sent:

You can create and manage your own Worker Pool dedicated to computation rather than the Node.js I/O-themed Worker Pool. The most straightforward ways to do this is using Child Process or Cluster.

and...

I/O-intensive tasks involve querying an external service provider (DNS, file system, etc.) and waiting for its response. While a Worker with an I/O-intensive task is waiting for its response, it has nothing else to do and can be de-scheduled by the operating system, giving another Worker a chance to submit their request.

and...

If you rely on only one Worker Pool, e.g. the Node.js Worker Pool, then the differing characteristics of CPU-bound and I/O-bound work may harm your application's performance.

For this reason, you might wish to maintain a separate Computation Worker Pool.

Node.js scales well, sometimes better than more heavyweight approaches like Apache. The secret to the scalability of Node.js is that it uses a small number of threads to handle many clients.

That would of course be very nice if in our case a thread with an RFC call waits for its response that this thread could process another RFC call in the meantime, as described above.

bsrdjan commented 2 years ago

Reading the link it should be indeed doable.

Considering efforts required for analysis, fundamental re-factoring and testing, the development is unlikely to start any time soon.

Using the environment variable the bottleneck is removed. If there is a business case or other advantages, to justify development efforts?

MrSJS commented 2 years ago

well, because the architecture of Node.js is like that.

Node.js scales well, sometimes better than more heavyweight approaches like Apache. The secret to the scalability of Node.js is that it uses a small number of threads to handle many clients. If Node.js can make do with fewer threads, then it can spend more of your system's time and memory working on clients rather than on paying space and time overheads for threads (memory, context-switching). But because Node.js has only a few threads, you must structure your application to use them wisely.

You may wish to distinguish between CPU-intensive and I/O-intensive tasks because they have markedly different characteristics. If you rely on only one Worker Pool, e.g. the Node.js Worker Pool, then the differing characteristics of CPU-bound and I/O-bound work may harm your application's performance. For this reason, you might wish to maintain a separate Computation Worker Pool.

And finally, for analytical purposes, I don't know whether uv_threadpool_size is big enough at all times during the run time. If it is not big enough, Node.js blocks all other activities in the meantime. That could unnecessarily cost performance unnoticed. And if I set too many threads, according to other articles on the net, it could also cost performance. Some articles write that we should only use as many threads in Node.js as we have logical cores in our hardware ... And that would currently not be enough, because each parallel RFC call needs an extra thread.

https://dev.to/johnjardincodes/increase-node-js-performance-with-libuv-thread-pool-5h10

bsrdjan commented 2 years ago

Yes, it might make sense and should be investigated in detail. Posted the question also in Node.js repository: nodejs/node-addon-api#1060. Unfortunately, considering current priorities and capacity allocated for node-rfc project, the re-factoring is unlikely to start soon.

If the inquiry is related to customer requirement or similar business case please contact me via LinkedIn.

bsrdjan commented 2 years ago

I will be on vacation till end of September but feel free to discuss with Node.js team, responding to above mentioned issue.

This issue stays open until the documentation is updated.

MrSJS commented 2 years ago

Ok thank you very much.

bsrdjan commented 2 years ago

Per https://github.com/nodejs/node-addon-api/issues/1060 outcome we keep the libuv.