-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Use ISapFunctionMetadata in InstallGenericServerFunctionHandler #53
base: develop
Are you sure you want to change the base?
Use ISapFunctionMetadata in InstallGenericServerFunctionHandler #53
Conversation
} | ||
// We keep a reference to the current server function delegates so they don't get GCed | ||
// Otherwise the application crashes as the delegate passed into native does no longer exists in managed | ||
#pragma warning disable SA1306 // Field names should begin with lower-case letter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like VS can't handle this correctly see dotnet/roslyn#23391
39cbde1
to
65dfaa8
Compare
@@ -121,5 +121,8 @@ private ISapExceptionMetadata GetExceptionByName(string name) | |||
|
|||
return new SapExceptionMetadata(excDesc); | |||
} | |||
|
|||
// Used by SapServer.InstallGenericServerFunctionHandler | |||
internal IntPtr GetFunctionDescHandle() => _functionDescHandle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep classes closed. In my book, internal properties like this create unwanted coupling. Would there be an other way to solve what you're trying to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not without exposing the _functionDescHandle
in the public interface or some other kind of internal coupling.
src/SapNwRfc/SapServer.cs
Outdated
@@ -149,31 +150,55 @@ private static RfcResultCode HandleGenericFunction(RfcInterop interop, Action<IS | |||
} | |||
} | |||
|
|||
private static RfcResultCode HandleGenericMetadata(RfcInterop interop, SapConnectionParameters parameters, string functionName, out IntPtr funcDescHandle) | |||
private static RfcResultCode HandleGenericMetadata(string functionName, RfcAttributes attributes, out IntPtr funcDescHandle, Func<string, SapAttributes, ISapFunctionMetadata> getFunctionMetadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the changes in this PR are solely to remove some duplication here, I would not merge it.
But I understand that it is to avoid opening connections each time? I wonder, do we have to get the metadata from the connection each time the server requests it, or could we get the metadata once and return that everytime the server asks for it. What would be the downside of this? (I never used server callbacks before, so please bear with me 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR exposes the getFunctionMetadata
to the user, so they have more control over retrieving the metadata. So they can:
- Handle exceptions on their own.
- Reuse
SapConnection
s. - Keep control over the
SapConnectionParameters
. - Use different connections depending on
functionName
andSapAttributes
. Which might be needed if there are multipleSapServer
instances connected to different SAP systems. SinceInstallGenericServerFunctionHandler
is static after all.
I missed the fact that So the idea is that |
Yes, Another way is to register server functions for specific 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
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 public static void InstallGenericServerFunctionHandler(string connectionString, Action<ISapServerConnection, ISapServerFunction> action) public static void InstallGenericServerFunctionHandler(SapConnectionParameters parameters, Action<ISapServerConnection, ISapServerFunction> action)
Unfortunately there isn't an API to register a server function for a specific SAP server instance... |
From what I understand from the sapnwrfc.h file (which is my only documentation at this point):
Now, in the callback (
Am I correct that, if we would register the server ourselves using If that is the case, would it be possible to boil down a given |
I am not sure that you can combine But I may have found another way this could work:
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;
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;
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. |
78ec3b8
to
3901a81
Compare
It looks like we can bind to A generic metadata handler is still needed though. |
3901a81
to
e5ff941
Compare
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? |
e5ff941
to
def838d
Compare
def838d
to
0e651de
Compare
@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. |
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. |
@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 |
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 |
@tom-j-irvine Thanks for testing! Regarding the feedback to the /** \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); |
Yeah, asking the server itself sounds a lot better than trying to keep track of it in code. |
I have implemented |
Works great. Thank you! |
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 theSapPooledConnection
.Should we deprecate the old API?Old API is removed.