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

Graceful handling for PulsarClient.CloseAsync #251

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

LeoSht
Copy link
Contributor

@LeoSht LeoSht commented Feb 10, 2024

Hi,
Current version of the client appears to rely on socket connection dispose call to break out of read operation when PulsarClient.CloseAsync is called. This leads to warnings in the log and prints out an exception like this:
System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'SslStream'.
...

These exceptions show up a few seconds after the PulsarClient.CloseAsync call was made, so don't easily show up on integration tests.

The proposed change supplies a CancellationToken to the PipeReader.ReadAsync method. And then triggers token cancellation on Dispose method call in ClientCnx.

Here is an example test (C#) to observe the fix, assuming we have an ILogger implementation that collects log statements as a string:

    [Fact]
    public async Task Client_Close_Graceful()
    {
        // Arrange
        var logTextBuilder = new StringBuilder();
        var logger = new StringLogger(logTextBuilder, LogLevel.Information);

        // Act
        var client = await GetClientAsync(logger);
        var topic = "public/default/test_graceful_client_close";
        var producer = await client.NewProducer()
            .Topic(topic)
            .CreateAsync();
        await producer.DisposeAsync();
        await client.CloseAsync();

        // Wait for socket timeout
        await Task.Delay(TimeSpan.FromSeconds(60));

        // Assert
        var logOutput = logger.Output.ToString();
        Assert.DoesNotContain("Exception", logOutput);
    }

Let me know if you need any additional information.
Regards,
Leo.

@LeoSht LeoSht changed the base branch from develop to 2.x February 10, 2024 04:18
@@ -831,7 +832,10 @@ and internal ClientCnx (config: PulsarClientConfiguration,
| Result.Error (UnknownCommandType unknownType), consumed ->
Log.Logger.LogError("{0} UnknownCommandType {1}, ignoring message", prefix, unknownType)
reader.AdvanceTo consumed
with Flatten ex ->
with
| ?: OperationCancelException ->
Copy link
Collaborator

@Lanayx Lanayx Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please match after (inside) flatten, not before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - done.

@Lanayx Lanayx merged commit 5db4ef0 into fsprojects:2.x Feb 12, 2024
2 checks passed
@Lanayx
Copy link
Collaborator

Lanayx commented Feb 12, 2024

Published 2.15.1

@LeoSht
Copy link
Contributor Author

LeoSht commented Feb 12, 2024

Thank you!

@LeoSht LeoSht deleted the close-client-gracefully branch February 13, 2024 00:17
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