Server support integration

[samuel-c-allan]

Creating a PR to hopefully receive more attention for this request. This is to address issue #209. The basic server functions work well and has been tested by several independent users (some of whom reached out to me as they needed a working server).

The code may need some cleaning up implemented (server destruct). Otherwise, I think it covers basic support decently enough.

I am prepared to make any changes as needed

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

:white_check_mark: samuel-allan
:white_check_mark: bsrdjan
:white_check_mark: samuel-c-allan
:x: sapcs

---

sapcs seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[samuel-c-allan]

I commited from a bunch of different accounts and have to ammend this mess now, still if you can look at the code in the meantime it would help

[bsrdjan]

Hello @samuel-c-allan, I was wondering why it takes so long but now I see :) Thank you very much for this great step forward with the server support.

I am back from vacation and was able to test and review the code. The test works and the solution looks good to me, can only imagine imagine the dedicated work and testing required to put all pieces together.

Here how I tested, just as a reference for the documentation and troubleshooting:

server.js

const addon = require("../lib");
const Server = addon.Server;
const server = new Server({ dest: "gateway" }, { dest: "MME" });

// Callback function (old)
// function my_stfc_connection(request_context, REQUTEXT = "") {
//     console.log("stfc invoked");
//     console.log("request_context", request_context);
//     console.log("abap_parameters", abap_parameters);

//     return {
//         ECHOTEXT: REQUTEXT,
//         RESPTEXT: `Node server here. Connection attributes are:\nUser '${user}' from system '${sysId}', client '${client}', host '${partnerHost}'`,
//     };
// }

// Callback function
function my_stfc_connection(error, abap_parameters, done) {
    console.log("NodeJS stfc invoked ", abap_parameters, error);

    done(
        {
            REQUTEXT: abap_parameters.REQUTEXT,
            ECHOTEXT: abap_parameters.REQUTEXT,
            RESPTEXT: `~~~ ${abap_parameters.REQUTEXT} ~~~`,
        },
        (err) => {
            if (err)
                console.log(
                    "Error occurred while transferring data to ABAP!",
                    err
                );
            else console.log("All fine :)");
        }
    );
}

server.start((err) => {
    if (err) return console.error("error:", err);
    console.log(
        `Server alive: ${server.alive} client handle: ${server.client_connection} server handle: ${server.server_connection}`
    );

    // Expose the my_stfc_connection function as RFM with STFC_CONNECTION pararameters (function definition)
    const RFM_NAME = "STFC_CONNECTION";
    server.addFunction(RFM_NAME, my_stfc_connection, (err) => {
        if (err) return console.error(`error adding ${RFM_NAME}: ${err}`);
        console.log(`Serving ${RFM_NAME}`);
    });
});

// let server serve
setTimeout(function () {
    console.log("bye!");
}, 20 * 1000);

ZSERVERTEST.abap

*&---------------------------------------------------------------------*
*& Report ZSERVERTEST
*&---------------------------------------------------------------------*
*&
*&---------------------------------------------------------------------*
REPORT zservertest.

DATA lv_echo LIKE sy-lisel.
DATA lv_resp LIKE sy-lisel.

CALL FUNCTION 'STFC_CONNECTION' DESTINATION 'NODEJS'
  EXPORTING
    requtext = 'XYZ'
  IMPORTING
    echotext = lv_echo
    resptext = lv_resp.

WRITE lv_echo.
WRITE lv_resp.
WRITE 'Bye!'.

NodeJS system

node ci/server.js
Server alive: true client handle: 140324433551872 server handle: 140324433760768
Serving STFC_CONNECTION
NodeJS stfc invoked  { ECHOTEXT: '', RESPTEXT: '', REQUTEXT: 'XYZ' } undefined
All fine :)
bye!

ABAP report

Program ZSERVERTEST
------------------------------
XYZ
~~~ XYZ ~~~
Bye!

The data transfer works in both directions and the only gap I noticed is that request_context is not passed from ABAP to NodeJS callback function. Did you maybe investigate how to pass the request_context because having it can be very helpful in NodeJS functions ?

But even so I would adopt the PR because the full cycle works, with data transfer in both directions. To merge the PR the contributor agreement shall be signed and the GitHib shows it is not signed yet.

license/cla Pending — Contributor License Agreement is not signed yet.

Thank you also for the memory leak fix. On which platform did you observe it first?

[samuel-c-allan]

Dear @bsrdjan thank you for the comment. As an aside I've just fixed some conflicts but I think I may have messed up the package-lock.json. Would appreciate if you could give it a quick check.

Re time - truth be told I had a working (but very messy) solution within a week and a half of initial coding but it was a combination of factors that led to the delay.. Also there was a misunderstanding - I thought only bots submitted PRs and that the issue had to be "approved" or something and just waited for that until I realized that nothing like that had to be done (and to be fair to me there was also delay in reviewing my code, which I completely understand given the workload of the team here). Either way we're here now and I'd be happy to extend the functionality further.

There are currently a few things which need further correcting on which I would appreciate your input:

1. Throughout this integration I've had a lot of trouble finding good comprehensive documentation for napi and because of this a lot of things feel a bit weird. For example I am uncertain as to Napi::Persistent, I am honestly a little confused about when exactly you might want to use it. I noticed on one occasion JS kept destroying an object I wanted alive and I used it there, but a fuller understanding would be appreciated (also a link to memory internals of V8 would be awesome as I could actually understand that instead of feeling in the dark)
2. The server does not currently have proper destruction logic, but this should be fairly trivial to implement

If you are curious re when I found the leak it was basically when running enormous jobs (which we tend to do at our company) I noticed memory usage was increasing and spiking. I then ran valgrind (which was a horror, I had to increase the call trace size to like 30 or more as far as I remember) and painfully located where it started. I'm still not sure why JS doesn't actually manage it on its own but for whatever reason it doesn't (I recall I even figured out why but I do not want to return to that part of the code just to figure it out again and type it out).

[samuel-allan]

Update: server.stop now fully cleans everything up (in fact you can start and stop and start and stop any number of times, I just wrote a program that does that every 10 seconds and you get the expected appearing and disappearing entry in SMGW)

[bsrdjan]

package-lock.json. Would appreciate if you could give it a quick check

Sure, will check at the end

Regarding Napi::Persistent this link helped me start understanding it: https://nodejs.github.io/node-addon-examples/special-topics/object-function-refs/, also these examples: https://github.com/nodejs/node-addon-examples When in doubt I also asked questions in node-addon-api issues and search them for possible solutions. The documentation is indeed scattered.

Regarding the destruction logic, thank you for adding it as well, it was missing.

Could you have a look into adding request_context argument to server function call? It will be likely required in many scenarios.

[samuel-allan]

Regarding Napi::Persistent this link helped me start understanding it: https://nodejs.github.io/node-addon-examples/special-topics/object-function-refs/, also these examples: https://github.com/nodejs/node-addon-examples When in doubt I also asked questions in node-addon-api issues and search them for possible solutions. The documentation is indeed scattered.

Thanks for the links, very helpful.

Could you have a look into adding request_context argument to server function call? It will be likely required in many scenarios.

Absolutely, I will update the callback with this parameter in one of my next commits

[samuel-allan]

I think I found how we can get the request_context. Just to be clear you mean the context of a stateful server right? Here is a snippet from the rfc sdk programming guide:

RfcGetServerContext(conn, &context, errorInfo);
if (errorInfo->code !=RFC_OK)
    return errorInfo->code;

// Set connection stateful on first incoming call.
if (!context.isStateful)
    RfcSetServerStateful(conn, 1, errorInfo);

For reference see programming guide page 63.

[bsrdjan]

Not exactly the stateful server, the simplest context of the ongoing client request would be enough. It can extend the later on, as needed.

Here the background.

When the JS server/callback function is invoked on server system, it might be helpful to know from which user, system etc. the request is coming from. That information can be passed from server to callback function and this Python implementation could be used as reference.

The basic request context (can be extended later on) contains only connection attributes, obtained by RfcGetConnectionAttributes:

genericHandler

https://github.com/SAP/PyRFC/blob/main/src/pyrfc/_pyrfc.pyx#L1333

        rc = RfcGetConnectionAttributes(rfcHandle, &attributes, &errorInfo)
        if rc != RFC_OK:
            _server_log("genericHandler", "Request for '{func_name}': Error while retrieving connection attributes (rc={rc}).".format(func_name=func_name, rc=rc))
            if not server.debug:
                raise ExternalRuntimeError(message="Invalid connection handle.")
            conn_attr = {}
        else:
            conn_attr = wrapConnectionAttributes(attributes)
            _server_log("genericHandler", "User '{user}' from system '{sysId}', client '{client}', host '{partnerHost}' invokes '{func_name}'".format(func_name=func_name, **conn_attr))

        # Context of the request. Might later be extended by activeParameter information.
        request_context = {
            'connection_attributes': conn_attr
        }

That request context is then sent as a parameter to client JS callback function https://github.com/SAP/PyRFC/blob/main/src/pyrfc/_pyrfc.pyx#L1362

        # Invoke callback function
        result = callback(request_context, **func_handle_variables)

The signature of callback JS function can be something like:

function my_stfc_connection(request_context, error, abap_parameters, done) {
    const ctx = request_context;
    console.log("NodeJS stfc invoked ", abap_parameters, error);
    console.log(`User ${ctx.user} from system ${ctx.sysId}, client ${ctx.client}, host ${ctx.partnerHost} ...`)

Not sure if mentioned in the programming guide but has been requested by first test power users.

[samuel-allan]

Thanks for the info, that's all I need to implement it. The only thing is I would vote to keep the request_context as the last parameter as some users may not need it. So my_stfc_connection (error, abap_parameters, done, req_ctx), in that case some users may easily do my_stfc_connection (err, params, done) without bothering with the last one.

What do you think?

[bsrdjan]

Agree that the last one is better. Any other than first is better I think because of error-first JavaScript convention.

[samuel-c-allan]

We are working on lots of internal changes. I will recreate this PR once we are finished with them, for now I am closing it

[bsrdjan]

Would you mind to create one separate PR for the memory leak fix only?

[samuel-allan]

Ok sure

On Tue 2 Nov 2021, 00:16 Srdjan Boskovic, @.***> wrote:

Would you mind to create one separate PR for the memory leak fix only?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/SAP/node-rfc/pull/236#issuecomment-957088363, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPVR5HQNC37MDW65BCAWM3UJ5Q3BANCNFSM5ERKVWGA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[bsrdjan]

@samuel-c-allan any update on progress with the PR?

[bsrdjan]

server bindings provided as of release 3.3, feedback welcome