Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Status of server support #209

Closed
samuel-c-allan opened this issue Apr 15, 2021 · 33 comments
Closed

Status of server support #209

samuel-c-allan opened this issue Apr 15, 2021 · 33 comments

Comments

@samuel-c-allan
Copy link

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
Copy link
Member

bsrdjan commented Apr 15, 2021

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
Copy link

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
Copy link
Author

@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
Copy link
Author

@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
Copy link

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
Copy link
Author

samuel-c-allan commented Apr 15, 2021 via email

@gregorwolf
Copy link

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

@samuel-c-allan
Copy link
Author

samuel-c-allan commented Apr 15, 2021 via email

@gregorwolf
Copy link

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
Copy link
Author

samuel-c-allan commented Apr 15, 2021 via email

@samuel-c-allan
Copy link
Author

samuel-c-allan commented Apr 15, 2021

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

  • 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
  • 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
  • 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
Copy link
Member

bsrdjan commented Apr 16, 2021

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
Copy link
Author

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 😄

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
Copy link
Author

@samuel-c-allan
Copy link
Author

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:

  1. Input data is converted to JS using the functions already in place
  2. 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]
  3. The genericHandler then waits on the uv_cond [2]
  4. 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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@bsrdjan
Copy link
Member

bsrdjan commented Apr 18, 2021

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
Copy link
Author

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
Copy link
Author

(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
Copy link
Author

Nvm I had stupid settings in SM59. Apologies

@samuel-c-allan
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Author

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

@samuel-c-allan
Copy link
Author

@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
Copy link
Member

bsrdjan commented Sep 25, 2021

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

@KunalBurangi
Copy link

KunalBurangi commented Sep 12, 2022

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

@bsrdjan
Copy link
Member

bsrdjan commented Sep 29, 2022

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
Copy link
Author

samuel-c-allan commented Jan 26, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants