Status of server support

samuel-c-allan commented 3 years ago

This issue is mostly a question but also a comment on documentation.

The team I work with desperately needs server support in our application (which is currently heavy on use of node-rfc) but I am currently finding it very hard to tell if (and to what extent) ABAP-to-JS calling is supported as the README is very unclear about this.

Inspection of Server.cc and some tests with the example experimental code have led me to believe that the basic connectivity is supported but the JS callback has not yet been implemented (in fact there is some commented out code in Server.cc that seems to be intended to support this).

Due to the urgency of our requirement I will certainly be implementing the missing functions in C++, but if the opportunity allows I would greatly wish to contribute to work on the server functionality or in the case that it is already implemented and I am misusing it, to understand what is going wrong. Except the following prevents this (and this is the main complaint of my issue): It is very much unclear from the README where the current work on the server stands, what is supported and what is up for contribution (there is a months old server branch which also raises more questions than it answers.

Looking forward to any clarification on the matter and again, I would certainly be willing to contribute to this.

bsrdjan commented 3 years ago

Hello @samuel-c-allan,

the sever functionality is not yet available and please ignore the old server branch, is just a leftover to be removed.

The commented code Server.cc#173 are three main steps of processing ABAP to Node RFC call:

1) ABAP RFC input data conversion to JS data 2) JS server function call (with JS data) 3) JS server function results conversion to ABAP RFC output data

The cycle is basically "inverted" client request cycle, in which steps 1) and 3) are exchanged. In client case JS data are converted to ABAP RFC input and ABAP RFC results is converted back to JS. These conversions were implemented for Client scenario and after (bigger) re-factoring re-used for Server data conversions.

The step 2), one-liner JS function call at line 183, is also implemented, leading to legitimate question, with all functions in place what is actually missing?

ABAP RFC call comes from external system, registered by node-rfc server and the handler section with commented code is invoked. ABAP system is external independent system from NodeJS perspective and ABAP RFC call is registered in a separate thread. All works fine, as long the handler does not touch JS data. With these handler steps un-commented the exception is raised because any JS access data must be synchronised with v8 event loop.

The contribution could help with adding the synchronisation, following:

I would be glad to help with more info, exchange or test env configuration.

gregorwolf commented 3 years ago

Dear @samuel-c-allan,

why would you need to use RFC for calling a JS functionality instead of using the ABAP HTTP Client to call a HTTP Endpoint implemented with NodeJS?

Best regards Gregor

samuel-c-allan commented 3 years ago

@gregorwolf This would be a good solution but unfortunately we have to work with very old systems which do not support the http client (namely 4.6c), plus I am happy for an opportunity to contribute to a project I use so extensively :)

samuel-c-allan commented 3 years ago

@bsrdjan Thank you for the awesomely detailed descriptions, I will thoroughly inspect all the code and the thread safe call example and get back with any questions / suggestions / whatever I find. I am very excited to be able to work on this part of node-rfc and further my understanding of V8 in the process.

gregorwolf commented 3 years ago

Just out of curiosity: The extended support for SAP R/3 4.6C ended 31.03.2013. So how is that system supported to run in a productive environment?

samuel-c-allan commented 3 years ago

Very valid question. Our product is intended to extract information from these older systems in order to help take them offline and you would be surprised how many people actually use these systems for better or for worse.I am sorry if I can't really explain it properly, I am just uncertain to what extent I am permitted to reveal the functionality at this stage (obviously later on the entire framework will be known).

gregorwolf commented 3 years ago

Interresting. But woudn't an extraction on DB level be easier to acomplish?

samuel-c-allan commented 3 years ago

Depends on what you mean with a db level extraction. If you mean a non-ABAP mediated extraction, this would possibly work but many of our clients have refused this approach for all sorts of internal reasons like preventing access to sensitive data, etc so at the end of the day that's the real reason (many clients will refuse running even harmless native sql). I myself am frustrated at many of the restrictions imposed that complicate the approach but I think the server method will be the cleanest way of achieving all this and in the worst case it isn't a horrible way of doing it and will certainly work, it is the cleanest of the dirty ways of achieving the result as far as I can see.

gregorwolf commented 3 years ago

But in regards to access to sensitive data there is no difference between native SQL and Open SQL in ABAP. Nothing restricts a developer.

samuel-c-allan commented 3 years ago

Absolutely. Now try and convince our clients and I will immediately switch to the much more sensible approches :P As I said, frustrating for someone with knowledge and understanding since I am aware of how much simpler and faster things could potentially be

samuel-c-allan commented 3 years ago

Alright so I read up on all the materials and have this basic plan of action in mind (step by step):

[x] Create a proof-of-concept version of round_trip (the same prime generating program) that has a synchronous system closer to what is expected here using libuv condition variables. This may be awkward for the primes program but will ensure we are on the same page given my limited experience with libuv
[x] Carry over the concepts from round_trip over to the server logic here, replacing ThreadItem with a version better suited to storing the request/response information
[x] Implement the wait for some condition variable after calling the JS function and a notify after the JS function resolves/returns (is a promise-based system better here? Or just notify whenever the function returns?)

This seems to be the basic idea, as you can probably tell I haven't worked with all of these technologies extensively in the past but if this plan sounds good I will go ahead with step 1 (adapting round_trip).

bsrdjan commented 3 years ago

It looks good to me. The simulation in step 3 could be eventually avoided and replaced by local RFC server test, using node-rfc client to call node-rfc server. From node-rfc server perspective there is no difference, it anyway does not "see" if ABAP or node client is on another side of RFC connection. The setup was not possible at the time when I was working on this but with WebSockets RFC support added in NWRFC SDK PL6 the local test should be possible. Will check that.

Reading about the extraction scenario, would it be easier to use the RFC client to pull the data from ABAP system? Extraction using node-rfc server looks like more efforts to me because of (custom?) ABAP reports required to to extract and push the data to node? But if already existing reports can be consumed then it is of course easier.

samuel-c-allan commented 3 years ago

Reading about the extraction scenario, would it be easier to use the RFC client to pull the data from ABAP system? Extraction using node-rfc server looks like more efforts to me because of (custom?) ABAP reports required to to extract and push the data to node? But if already existing reports can be consumed then it is of course easier.

That is the model we have been using so far and there are a few reasons I want to switch (mostly a very particular performance problem with large tables). Basically just trust me it isn't an XY problem :smile:

Also, quick update regarding the whole thing - I have run a preliminary test of all the concepts and all appears to be working so hopefully within a day I will be able to link a testing branch which accomplishes the connection

samuel-c-allan commented 3 years ago

P.S. Just in case, this page was immensely useful: https://nodejs.org/dist/latest-v11.x/docs/api/n-api.html#n_api_asynchronous_thread_safe_function_calls

samuel-c-allan commented 3 years ago

Alright so I think I finally have everything laid out in my head. As I was working on this I realized this needs to handle the important case of several requests pouring in at the same time faster than the execution time of the JS callback. I think however that if my understanding of the napi_threadsafe_function is correct, this should be handled automatically in the following scheme:

First of all, when the server is being started (as far as I understand this happens in the native thread), a thread safe function and context are created and stored somewhere in the server class (as properties).

Then, when genericHandler gets called (which happens on an independent thread), the following happens:

Input data is converted to JS using the functions already in place
The thread-safe callback is called with the context which contains at the very minimum the request info, space for the response info and a uv_cond and associated mutex which are specific for the current instance of genericHandler [1]
The genericHandler then waits on the uv_cond [2]
Once the wait is over (which happens when the JS callback signals), serialize the JS response to ABAP and send back (You mentioned this is also handled by existing functions so I am not even thinking about this stage)

Now the obvious question this begs is how does JS signal that the uv_cond "resolved"? I think this should not be left up to the user and that the callback which is currently "raw" should be wrapped so that the real callback resides in src/ts/wrapper/sapnwrfc-server.ts and that real callback manages all the signalling whereas the "fake" user callback is actually called by this real callback before and after all the necessary steps.

Since JS can't really be expected to do the signalling directly (obviously) this probably means some function needs to be exposed to sapnwrfc-server.ts such as signal_back or something like that.

It seems to me this is a complete solution. Let me know what you think of this and I will begin implementing it shortly

[1] this is to allow several genericHandlers on separate threads to all call and wait for their own calls to the callback to complete [2] Probably need some clean strategy to avoid the false firings of uv_cond_wait that libuv warns against

samuel-c-allan commented 3 years ago

Quick update - I made the callback respond successfully. Now making sure all the memory is managed properly and modifying the architecture slightly to be a bit cleaner

samuel-c-allan commented 3 years ago

Update - requests are now fully functional and the thread-safe javascript callback is called successfully. Return values from javascript are currently ignored (so JS->ABAP serialization is not happening at the moment, but this is an easy fix once you confirmed everything works).

Please check out a comparison with the testing branch here. I didn't put enough effort into making the commits legible so I will break down the changes.

Category 1: Changes to the existing codebase

Server.cc: the _ServerFunctionStruct type was modified not to store regular js function callbacks but to store instances of a thread-safe callback instead
Server.cc: the destructor of _ServerFunctionStruct and addFunction and removeFunction were modified to work with TypedThreadSafeFunction
getRfmParameters was modified to take an optional env because before it would create everything with an assumed environment of main thread which would lead to a segfault. In all regular cases it will behave as usual, but in the special case of CallJs (see below) where a custom env is required, it will use this parameter.

Category 2: New stuff

Server.cc: ABAP to JS serialization has been moved away and into a CallJs method (see below). Instead genericHandler organizes a thread-safe call to CallJs (via BlockingCall) and waits on a condition variable until CallJs has finished
Server.cc: CallJs function that now handles the serialization of ABAP->JS and resolves the condition variable once all is finished
Server.h: Added types like TSFN (alias for thread-safe function with all the correct stuff set up) and DataType (just a struct holding temp information to be exchanged between genericHandler and CallJs

Testing!!!

So, I have only currently tested this with the following setup: I manually compile a sapnwrfc.node, create a function module (like in the example in README) to call STFC_CONNECTION with destination NODEJS and use the following javascript which has worked successfully for me at least:


//const noderfc = require('node-rfc');
const noderfc = require('./sapnwrfc.node');

let server = new noderfc.Server({DEST: 'gateway'}, {DEST: 'client'});

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}`
    );

    const RFM_NAME = "STFC_CONNECTION";
    server.addFunction(RFM_NAME, (a, b) => {
      console.log('YAY');
      console.log(a, b);
    }, (err) => {
      console.log(err);
      if (err) return console.error(`error adding ${RFM_NAME}: ${err}`);
    });

    console.log('Out of scope');
});

setTimeout(function () {
    console.log("bye!");
}, 10000 * 1000);

After testing you should get the following (pretty messy) output:

Metadata lookup for: STFC_CONNECTION
genericHandler for: STFC_CONNECTION func_handle: 139824321384656
found func_desc 96020096
Before cond [native thread]
YAY
undefined { ECHOTEXT: '', RESPTEXT: '', REQUTEXT: 'Hello Nöde 1' }
After cond [native thread]

So all of this is very raw, but I would be glad if you could look over it / test it and then we can move onto the next stage which would completing the cycle with JS->ABAP serialization and changing any type/variable names or other stuff to your liking.

samuel-c-allan commented 3 years ago

Also - there are some places with questionable memory management, bare with me I am fixing these

bsrdjan commented 3 years ago

Good and fast progress, many thanks for the update! Will do my best to have a look this week.

Could you please also sign the Contribution Agreement, if not already done: https://github.com/SAP/node-rfc#contributing

samuel-c-allan commented 3 years ago

Good and fast progress, many thanks for the update! Will do my best to have a look this week.

Could you please also sign the Contribution Agreement, if not already done: https://github.com/SAP/node-rfc#contributing

Quick unrelated question (it is a bit too simple for me to raise a separate issue so asking here) - there is an annoying feature that when I use a .node file compiled using npm run addon it creates .trc files every time the server receives a request containing a dump of the entire request load. This is totally fine regarding the functionality but I can't help but think I'm losing performance in my application because the process is writing the entire request (which is quite large in my case) to a file every time I receive a request. Is there some simple way I can disable this? Is it some CMake switch?

samuel-c-allan commented 3 years ago

(I mean is there a simple way to disable trace files. I did try TRACE=0 in ini but to no avail)

samuel-c-allan commented 3 years ago

Nvm I had stupid settings in SM59. Apologies

samuel-c-allan commented 3 years ago

I have finished it and cleaned up the code. There are still gaps in server support mostly when it comes to stopping the server (StopAsync is not implemented yet but this is not hard to do). Still, I have implemented the ABAP->JS and JS->ABAP flows and have successfully tested the former with enormous amounts of data for long periods of time and it doesn't fall over

samuel-c-allan commented 3 years ago

P.S. I played around with some of your methods and forgot to revert the changes, I will be reverting those shortly but the rest should be enough to give you an idea

samuel-c-allan commented 3 years ago

Also, important: When the JS callback is called from genericHandler it is passed 3 arguments (err, obj and done). The final argument refers to the function that I expect the user to call when he is ready for ABAP to continue executing. If the user wants to pass data to ABAP they can call this function with an object (e.g. done({abc: 123})).

I really needed it to be possible to call this function later for my own purposes and wasn't sure what way would best fit with the philosophies of the library. I did my best, let me know if anything needs to change.

samuel-c-allan commented 3 years ago

@bsrdjan Also if you do get to testing I might as well update the test code for you because the one I linked before is now out of date. Here it is:

//const noderfc = require('node-rfc');
const noderfc = require('./sapnwrfc.node');

let server = new noderfc.Server({DEST: 'gateway'}, {DEST: 'client'});

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}`
    );

    const RFM_NAME = "STFC_CONNECTION";
    server.addFunction(RFM_NAME, (err, obj, done) => {
      // err refers to any error, obj contains any data ABAP sent and done is the function to call when done
      console.log(obj);
      done({abaptest: 'yes'});
    }, (err) => {
      console.log(err);
      if (err) return console.error(`error adding ${RFM_NAME}: ${err}`);
    });

    console.log('Out of scope');
});

setTimeout(function () {
    console.log("bye!");
}, 10000 * 1000);

samuel-c-allan commented 3 years ago

Hi - apologies for all the excessive notifications. I have made some more changes to the API so in order not to have to summarize all here I have simply updated doc/usage.md in my testing branch. The latest usage examples can be seen there - please ignore all my other examples in this thread.

Looking forward to feedback.

samuel-c-allan commented 2 years ago

I have synced smarterpsoft/node-rfc master with SAP/node-rfc master (with changes in the testing branch integrated).

samuel-c-allan commented 2 years ago

@bsrdjan If you can, please let me know what the status is on this right now. Are you interested in this integration or not? I will understand if you are not but would like a notification in this case because I will separate the fork in that case and implement some other features

bsrdjan commented 2 years ago

Sorry for the delayed response, I am on vacation and can have a look earliest the end of next week.

KunalBurangi commented 1 year ago

What happens when the server disconnects? How does reconnect work? @samuel-c-allan

bsrdjan commented 1 year ago

I also did not get the latest status from @samuel-c-allan, not sure if he made the progress here, complete the implementation or faced some issue. I don't have a capacity atm for implementing the feature.

samuel-c-allan commented 1 year ago

Dear @bsrdjan, my apologies for responding so late to this.

I worked on the server when the company I work for needed it as part of a tool we were building (we have since switched to using nwrfc directly from C). Unfortunately as a result of some decisions made by management it is impossible for me to share that code, although if that restriction ever gets cleared I would be very happy to upload it (and I will try and see whether it still applies or not this week).

From: Srdjan Boskovic @.> Sent: Thursday, September 29, 2022 7:56 AM To: SAP/node-rfc @.> Cc: Samuel Allan @.>; Mention @.> Subject: Re: [SAP/node-rfc] Status of server support (#209)

I also did not get the latest status from @samuel-c-allanhttps://github.com/samuel-c-allan, not sure if he made the progress here, complete the implementation or faced some issue. I don't have a capacity atm for implementing the feature.

— Reply to this email directly, view it on GitHubhttps://github.com/SAP/node-rfc/issues/209#issuecomment-1262172717, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ATHKTJIA2I2IF322RFF4EKTWAV7XDANCNFSM426UT6KQ. You are receiving this because you were mentioned.Message ID: @.***>

SAP / node-rfc