huysentruitw / SapNwRfc

SAP NetWeaver RFC library for .NET 5, .NET Core and .NET Framework
MIT License
148 stars 43 forks source link

Use ISapFunctionMetadata in InstallGenericServerFunctionHandler #53

Open campersau opened 2 years ago

campersau commented 2 years ago

Since we now have the APIs for getting metadata, the InstallGenericServerFunctionHandler could expose the callback for handling metadata. Which gives the user more control by allowing the use of different connections for retrieving metadata and connection pooling, ~which I have highlighted in the readme example~.

~This also exposes the metadata APIs on the SapPooledConnection.~

~Should we deprecate the old API?~ Old API is removed.

huysentruitw commented 2 years ago

I missed the fact that InstallGenericServerFunctionHandler was static 😅 (I have no experience with SapServer myself)

So the idea is that InstallGenericServerFunctionHandler is only called once in an application? I don't quite understand how you could have different SapServers with different connection parameters, but would then only call InstallGenericServerFunctionHandler once and also have to pass some connection parameters. Shouldn't the SapServer class register a server listener itself and encapsulate it and have a much more user friendly API for handling different function callbacks?

campersau commented 2 years ago

Yes, InstallGenericServerFunctionHandler is only called once in an application.

Another way is to register server functions for specific sysIds:

DECL_EXP RFC_RC SAP_API RfcInstallServerFunction(SAP_UC const *sysId, RFC_FUNCTION_DESC_HANDLE funcDescHandle, RFC_SERVER_FUNCTION serverFunction, RFC_ERROR_INFO* errorInfo);

But with that you also can't tie it to a single SapServer either, because you could have multiple SapServers for the same sysId but with different PROGRAM_IDs... (Which is the use case I use at work.)


I don't quite understand how you could have different SapServers with different connection parameters, but would then only call InstallGenericServerFunctionHandler once and also have to pass some connection parameters.

That is exactly what this PR is solving by adding:

public static void InstallGenericServerFunctionHandler(Func<string, SapAttributes, ISapFunctionMetadata> getFunctionMetadata, Action<ISapServerConnection, ISapServerFunction> action)

I just have kept the previous methods for backward compatibility, as the ISapFunctionMetadata wasn't available when I first added them which is why I did the hack with the SapConnectionParameters 😞 :

public static void InstallGenericServerFunctionHandler(string connectionString, Action<ISapServerConnection, ISapServerFunction> action)
public static void InstallGenericServerFunctionHandler(SapConnectionParameters parameters, Action<ISapServerConnection, ISapServerFunction> action)

Should we deprecate the old API?


Shouldn't the SapServer class register a server listener itself and encapsulate it and have a much more user friendly API for handling different function callbacks?

Unfortunately there isn't an API to register a server function for a specific SAP server instance... Maybe we could do it ourselves by using SYSID and PROGRAM_ID as an identifier but I am not sure if that works.

huysentruitw commented 2 years ago

From what I understand from the sapnwrfc.h file (which is my only documentation at this point):

RfcCreateServer simplifies the process of managing server connections ourselves by calling RfcRegisterServer internally. But this function returns RFC_SERVER_HANDLE instead of a server connection, which seems logic as that implementation handles reconnects itself when needed.

Now, in the callback (RFC_SERVER_FUNCTION) we get the RFC_CONNECTION_HANDLE):

typedef RFC_RC (SAP_API* RFC_SERVER_FUNCTION)(RFC_CONNECTION_HANDLE rfcHandle, RFC_FUNCTION_HANDLE funcHandle, RFC_ERROR_INFO* errorInfo);

Am I correct that, if we would register the server ourselves using RfcRegisterServer, which returns an RFC_CONNECTION_HANDLE, we would be able to match the server in the callback by that connection handle?

If that is the case, would it be possible to boil down a given RFC_CONNECTION_HANDLE to the RFC_SERVER_HANDLE ?

campersau commented 2 years ago

I am not sure that you can combine RfcCreateServer with RfcRegisterServer like that, because you would either need to pass the RFC_CONNECTION_HANDLE from RfcRegisterServer to the RFC_SERVER_HANDLE which I don't think you can or implement the connection / dispatch logic of RfcCreateServer yourself and don't use RfcCreateServer at all. Or maybe I couldn't follow your thoughts?


But I may have found another way this could work:

  1. Add a RfcAddServerSessionChangedListener and keep track of the sessionIDs

    DECL_EXP RFC_RC SAP_API RfcAddServerSessionChangedListener(RFC_SERVER_HANDLE serverHandle, RFC_SERVER_SESSION_CHANGE_LISTENER sessionChangeListener, RFC_ERROR_INFO* errorInfo);
    typedef struct _RFC_SESSION_CHANGE {
    SAP_UC sessionID[30 + 1];   ///< Session ID of the user session in question
    RFC_SESSION_EVENT event;    ///< What has been done with that session
    } RFC_SESSION_CHANGE;
  2. Call RfcGetServerContext inside the function handler to get the sessionID.

DECL_EXP RFC_RC SAP_API RfcGetServerContext(RFC_CONNECTION_HANDLE rfcHandle, RFC_SERVER_CONTEXT* context, RFC_ERROR_INFO* errorInfo);
typedef struct _RFC_SERVER_CONTEXT{
    RFC_CALL_TYPE type;                     ///< Specifies the type of function call. Depending on the value of this field, some of the other fields of this struct may be filled.
    RFC_TID tid;                            ///< If type is RFC_TRANSACTIONAL or RFC_QUEUED, this field is filled with the 24 digit TID of the tRFC/qRFC unit.
    RFC_UNIT_IDENTIFIER* unitIdentifier;    ///< If type is RFC_BACKGROUND_UNIT, this pointer is set to the unit identifier of the LUW. Note: the pointer is valid only during the execution context of your server function.
    RFC_UNIT_ATTRIBUTES* unitAttributes;    ///< If type is RFC_BACKGROUND_UNIT, this pointer is set to the unit attributes of the LUW. Note: the pointer is valid only during the execution context of your server function.
    unsigned isStateful;                    ///< Specifies whether the current server connection is processing stateful RFC requests (assigned permanently to one fixed ABAP user session).
    SAP_UC sessionID[33];                   ///< Contains a unique zero-terminated session ID, identifying the ABAP or external user session. Can be used in stateful servers to store session context in a hashmap.
}RFC_SERVER_CONTEXT;
  1. Find the SapServer which has the same sessID and invoke an eventhandler / function there.

It looks like it requires stateful server though.


https://support.sap.com/en/product/connectors/nwrfcsdk.html there is a SAP NetWeaver RFC SDK ProgrammingGuide at the bottom which has some examples.

campersau commented 2 years ago

It looks like we can bind to SapServer instances with RfcAddServerSessionChangedListener and RfcGetServerContext see https://github.com/huysentruitw/SapNwRfc/pull/53/commits/3901a8148356259adabbcae14648c8733a41446c

A generic metadata handler is still needed though.

huysentruitw commented 2 years ago

Excuse me for the large delay. I'm no longer working on this project for which I wrote this library, and I no longer have access to an SAP server. Can you give me an update about this PR, do you think it is till in good shape to be merged?

campersau commented 2 years ago

@huysentruitw I have rebased it based on #70 should be good to go now.

@tom-j-irvine since you are using the SapServer it would be great if you could try out this PR.

tom-j-irvine commented 2 years ago

Yes, I will give it a try. Hopefully I can report back in a day or two. Thanks a lot for working on this - this project has been very helpful.

campersau commented 2 years ago

@tom-j-irvine Thanks! The setup is a little different now see https://github.com/campersau/SapNwRfc-1/blob/sapserverfunctionmetadata/README.md#rfc-server-generic-handler As it should now be possible to use multiple SapServers with multiple SAP systems. The readme currently just demonstrates the simplest case though.

tom-j-irvine commented 2 years ago

Everything seems to work well in my project. I stopped (and fully disposed) the server multiple times, restarted and never had the GC issue creep back in. If nothing else, the API seems a little more intuitive now too.

One minor enhancement I can think of would be to add a ServerState property to the ISapServer that could be updated by the StateChange event. That would allow us check the state and not have to track it externally based on the event. By no means is this necessary, but would be a nice-to-have item.

campersau commented 2 years ago

@tom-j-irvine Thanks for testing!

Regarding the feedback to the ServerState property: There is actually a method called RfcGetServerAttributes which also returns the server state and some additional info. I would prefer to expose this method on ISapServer instead.

/** \struct _RFC_SERVER_ATTRIBUTES
* \ingroup autoserver
*
* Information about an RFC Server returned by RfcGetServerAttributes(). 
*/
typedef struct _RFC_SERVER_ATTRIBUTES{
    SAP_UC* serverName;         ///< This server's name as given when creating the server.
    RFC_PROTOCOL_TYPE type;     ///< This RFC server's type. Will be one of RFC_MULTI_COUNT_REGISTERED_SERVER or RFC_TCP_SOCKET_SERVER.
    unsigned registrationCount; ///< The current number of active registrations (in case of a Registered Server) or the maximum number of parallel connections the server will accept (in case of a TCP Socket Server).
    RFC_SERVER_STATE state;     ///< This server's state.
    unsigned currentBusyCount;  ///< The number of requests currently being processed.
    unsigned peakBusyCount;     ///< The maximum number of requests the server has been processing in parallel since it has been created.
} RFC_SERVER_ATTRIBUTES;

DECL_EXP RFC_RC SAP_API RfcGetServerAttributes(RFC_SERVER_HANDLE serverHandle, RFC_SERVER_ATTRIBUTES* serverAttributes, RFC_ERROR_INFO* errorInfo);
tom-j-irvine commented 2 years ago

Yeah, asking the server itself sounds a lot better than trying to keep track of it in code.

campersau commented 2 years ago

I have implemented RfcGetServerAttributes here https://github.com/huysentruitw/SapNwRfc/pull/53/commits/c60998d0dd70836c2e68b35032a600b15558fc03

tom-j-irvine commented 2 years ago

Works great. Thank you!