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

Keep reference to generic server function delegates so they don't get GCed and crash the application #70

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

campersau
Copy link
Contributor

Extracted the fix from #53

Fixes #56

@tom-j-irvine
Copy link

I'm looking forward to this making it into the library as I currently have it manually implemented in my code. I've run into one small thing while using it though and want to mention it. In my application, I need to be able to Stop/Start the RFC Server.

I can do a _server.Shutdown() and _server.Launch(), but if I fully dispose my ISapServer and re-create it, I run into the GC issue after launching my new instance. Generally, I think this is OK and works for my current purposes. However, it seems like the only way to really destroy the server and delegates is to fully recycle the application.

@campersau
Copy link
Contributor Author

@tom-j-irvine can you please show some code where you would run into the GC issue? Or are you referring to the implementation in #53 ?

@tom-j-irvine
Copy link

Here is what works:

private ISapServer _sapServer;

// ... lots of other code

private void StartRfcServer()
{
    // force shutdown if already running
    StopRfcServer(false);
    
    // start
    _logger.LogInformation("Starting RFC server");
    try
    {
        if (_sapServer == null) 
        {

            _sapServer = SapServer.Create(_config.GetRfcServerParameters());

            SapServer.InstallGenericServerFunctionHandler(
                _config.GetRfcDestinationParameters(),
                (connection, function) => HandleFunction(function)
                );
        }

        _sapServer.Launch();

        _logger.LogInformation("RFC server started");
    }
    catch (Exception ex)
    {
        _logger.LogError("Exception starting RFC server: {message}", ex.Message);
    }
    
}

private void StopRfcServer(bool logShutdown = true)
{
    if (_sapServer != null)
    {
        if (logShutdown) _logger.LogInformation("Stopping RFC server");
        try
        {
            _sapServer.Shutdown(2000);
            if (logShutdown) _logger.LogInformation("RFC server stopped");
        }
        catch (Exception ex)
        {
            _logger.LogWarning("Exception shutting down RFC server: {message}", ex.Message);
        }
    }
}

As I said, this works for my purposes. However, I had a version of the code where during shutdown, I would completely dispose the _sapServer and assign null back to it. Then, after starting it back up with Create and InstallGenericServerFunctionHandler, I would run into the GC issue.

I'm really not sure this matters, but there does seem to be something lingering if you create a server and try to destroy it. I imagine things could get funky if you tried to create multiple servers too, but hopefully no one would try to do that.

@campersau
Copy link
Contributor Author

Thanks! I will investigate it.

@campersau campersau force-pushed the keepreferencetosapserverdelegate branch from 8e21814 to f92a55c Compare August 24, 2022 20:13
@campersau campersau force-pushed the keepreferencetosapserverdelegate branch from f92a55c to 5b2f46a Compare August 24, 2022 20:23
@campersau
Copy link
Contributor Author

It looks like that calling RfcInstallGenericServerFunction multiple times does not work. So I changed the code that calling SapServer.InstallGenericServerFunctionHandler multiple times will only set the user delegates and RfcInstallGenericServerFunction is just called the first time. Which avoids wrapping the users methods but introduces a few more static fields.

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

Successfully merging this pull request may close these issues.

2 participants