Re: [PR] Fix issue with cancelling Receive method call [pulsar-dotpulsar]

["blankensteiner (via GitHub)" <gi...@apache.org>]

blankensteiner commented on code in PR #267:
URL: https://github.com/apache/pulsar-dotpulsar/pull/267#discussion_r2120400076


##########
tests/DotPulsar.Tests/Internal/ConsumerTests.cs:
##########
@@ -467,6 +468,41 @@ public async Task Connectivity_WhenConnectionIsInitiallyUpAndReconnects_ShouldBe
         exception.ShouldBeNull();
     }
 
+    [Fact]
+    public async Task Should_Not_Increase_Permits_On_TryReceive()
+    {
+        //Arrange
+        var topicName = await _fixture.CreateTopic(CancellationToken.None);
+        var subscription = CreateSubscriptionName();
+        var maxPrefetch = 2;
+
+        using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2));
+        using var httpClient = CreateAdminClient();
+        await using var pulsarClient = CreateClient();
+        await using var consumer = CreateConsumer(pulsarClient, topicName, subscription, Schema.ByteSequence, (uint)maxPrefetch);
+        await using var producer = CreateProducer(pulsarClient, topicName, Schema.ByteSequence);
+
+        await consumer.StateChangedTo(ConsumerState.Active, cts.Token);
+
+        // Wait until we get our first message
+        await producer.Send([1], cts.Token);
+        var message = await consumer.Receive(cts.Token);
+        await consumer.Acknowledge(message, cts.Token);
+
+        //Act
+        var maxPermits = 0L;
+        for (int i = 0; i < maxPrefetch * 5; i++)
+        {
+            consumer.TryReceive(out _).ShouldBe(false);
+            await Task.Delay(50, cts.Token);

Review Comment:
   Is this delay needed?



##########
tests/DotPulsar.Tests/Internal/ConsumerTests.cs:
##########
@@ -467,6 +468,41 @@ public async Task Connectivity_WhenConnectionIsInitiallyUpAndReconnects_ShouldBe
         exception.ShouldBeNull();
     }
 
+    [Fact]
+    public async Task Should_Not_Increase_Permits_On_TryReceive()

Review Comment:
   To follow the naming convention a good test name would be something like: TryReceive_WhenBufferIsEmpty_ShouldNotIncreasePermits



##########
tests/DotPulsar.Tests/Internal/ConsumerTests.cs:
##########
@@ -467,6 +468,41 @@ public async Task Connectivity_WhenConnectionIsInitiallyUpAndReconnects_ShouldBe
         exception.ShouldBeNull();
     }
 
+    [Fact]
+    public async Task Should_Not_Increase_Permits_On_TryReceive()
+    {
+        //Arrange
+        var topicName = await _fixture.CreateTopic(CancellationToken.None);
+        var subscription = CreateSubscriptionName();
+        var maxPrefetch = 2;
+
+        using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2));

Review Comment:
   Why not use the _cts? :-)



##########
tests/DotPulsar.Tests/Internal/ConsumerTests.cs:
##########
@@ -467,6 +468,41 @@ public async Task Connectivity_WhenConnectionIsInitiallyUpAndReconnects_ShouldBe
         exception.ShouldBeNull();
     }
 
+    [Fact]
+    public async Task Should_Not_Increase_Permits_On_TryReceive()
+    {
+        //Arrange
+        var topicName = await _fixture.CreateTopic(CancellationToken.None);

Review Comment:
   Let's pass _cts.Token to this method



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org