Stop Listener when disposed (#104) #105

alrod · 2022-02-03T02:27:59Z

Address issue detailed in #104

Also as part of the PR I added the track2 port (6c9df8b) which helps with Dispose_Calls_StopAsync test and CRIs invetigations in general:
Azure/azure-sdk-for-net@bad9b85

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

brettsam

This looks okay to me -- maybe @sidkri wants to double-check as well?

sidkri · 2022-02-08T11:38:44Z

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

+                    // Host for the EventHubListener have beed disposed,
+                    // the invocation(s) can be uncompleted in this case
+                    // and we do not to checkpoint to avoid possible data lose
+                    _logger.LogDebug(GetOperationDetails(context, "SkipCheckpointDueHostDisposed"));


nit: SkipCheckpointDueToHostDisposed

sidkri · 2022-02-08T11:45:39Z

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

                {
-                    await CheckpointAsync(context);
+                    // Host for the EventHubListener have beed disposed,
+                    // the invocation(s) can be uncompleted in this case


Rather than saying "uncompleted", could you comment that we expect function invocations to fail before reaching customer code and so the checkpoint should not be advanced?

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

sidkri · 2022-02-08T11:56:02Z

test/Microsoft.Azure.WebJobs.Extensions.EventHubs.Tests/EventHubApplicationInsightsTest.cs

@@ -308,7 +308,7 @@ public async Task ProcessEvents_SingleDispatch_SystemPropertiesAndApplicationPro
            var loggerMock = new Mock<ILogger>();
            var executor = new Mock<ITriggeredFunctionExecutor>(MockBehavior.Strict);
            executor.Setup(p => p.TryExecuteAsync(It.IsAny<TriggeredFunctionData>(), It.IsAny<CancellationToken>())).ReturnsAsync(new FunctionResult(true));
-            var eventProcessor = new EventHubListener.EventProcessor(options, executor.Object, loggerMock.Object, true, checkpointer.Object);
+             var eventProcessor = new EventHubListener.EventProcessor(options, executor.Object, loggerMock.Object, true, new Mock<EventHubListener.IDisposed>().Object, checkpointer.Object);


nit: indenting

sidkri · 2022-02-08T12:06:40Z

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

@@ -88,9 +91,11 @@ public async Task StopAsync(CancellationToken cancellationToken)
            _started = false;


Is there any possibility of StopAsync() getting called concurrently ? Not sure what happens if UnregisterEventProcessorAsync() is called concurrently. We may want to add a lock around line this call.

sidkri

Requesting a discussion around my comment on line 75 of EventHubListener before merging this PR.

sidkri · 2022-02-11T20:22:11Z

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

        private bool _started;
+        private string _details;


sidkri · 2022-02-11T20:26:07Z

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs

            }
-            _started = false;
+
+            _logger.LogDebug($"EventHub listener stopped ({_details})");


Move this within the if (_started) block so we don't log on a no-op call to StopAsync() (e.g. when StopAsync is called for the second time)

Alternately, its fine if we want to show how many times StopAsync() was called. But in this case, we may want to log "stopped" in the if (_started) block and a different log statement in the else condition to indicate no action was needed as the listener is already stopped. This will give us better insights when debugging issues.

sidkri · 2022-02-11T20:37:01Z

test/Microsoft.Azure.WebJobs.Extensions.EventHubs.Tests/EventHubListenerTests.cs

+            string functionId = "FunctionId";
+            string eventHubName = "EventHubName";
+            string consumerGroup = "ConsumerGroup";
+            var storageUri = new Uri("https://eventhubsteststorageaccount.blob.core.windows.net/");


not used, hard coded on line 246 instead. consider moving this to a class member as the same URI is used in GetMonitor_ReturnsExpectedValue() as well.

alrod requested a review from brettsam February 3, 2022 02:28

alrod force-pushed the alrod/fix-disposed-race branch from f5cc60e to 8a1fd20 Compare February 3, 2022 20:36

brettsam reviewed Feb 4, 2022

View reviewed changes

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs Show resolved Hide resolved

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs Outdated Show resolved Hide resolved

alrod requested a review from brettsam February 4, 2022 19:00

brettsam previously approved these changes Feb 4, 2022

View reviewed changes

alrod mentioned this pull request Feb 4, 2022

Checkpoint progressed on internal azure function exceptions #66

Closed

sidkri reviewed Feb 8, 2022

View reviewed changes

src/Microsoft.Azure.WebJobs.Extensions.EventHubs/Listeners/EventHubListener.cs Outdated Show resolved Hide resolved

sidkri reviewed Feb 8, 2022

View reviewed changes

sidkri suggested changes Feb 8, 2022

View reviewed changes

ghost added Needs: author feedback and removed Needs: author feedback labels Feb 8, 2022

sidkri linked an issue Feb 9, 2022 that may be closed by this pull request

Stop Listener when disposed #104

Closed

alrod dismissed brettsam’s stale review via 2787f67 February 10, 2022 22:26

alrod force-pushed the alrod/fix-disposed-race branch from 8a1fd20 to 2787f67 Compare February 10, 2022 22:26

alrod requested review from brettsam and sidkri February 10, 2022 22:28

Stop Listener when disposed (#104)

6d84ee8

alrod force-pushed the alrod/fix-disposed-race branch from 2787f67 to 6d84ee8 Compare February 10, 2022 22:30

Add listener details

46b283d

alrod force-pushed the alrod/fix-disposed-race branch from 6c9df8b to 46b283d Compare February 11, 2022 00:54

liliankasem previously approved these changes Feb 11, 2022

View reviewed changes

sidkri reviewed Feb 11, 2022

View reviewed changes

sidkri previously approved these changes Feb 11, 2022

View reviewed changes

Fix comments

afa89ff

alrod dismissed sidkri’s stale review via afa89ff February 11, 2022 22:00

alrod dismissed liliankasem’s stale review via afa89ff February 11, 2022 22:00

liliankasem approved these changes Feb 11, 2022

View reviewed changes

alrod merged commit 3a6f3ee into dev Feb 11, 2022

cachai2 mentioned this pull request Feb 16, 2022

[BUG] Listeners may be Disposed before StopAsync() is called Azure/azure-sdk-for-net#27056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop Listener when disposed (#104) #105

Stop Listener when disposed (#104) #105

alrod commented Feb 3, 2022 •

edited

Loading

brettsam left a comment

sidkri Feb 8, 2022

sidkri Feb 8, 2022

sidkri Feb 8, 2022

sidkri Feb 8, 2022

sidkri left a comment

sidkri Feb 11, 2022

sidkri Feb 11, 2022

sidkri Feb 11, 2022 •

edited

Loading

sidkri Feb 11, 2022

		@@ -88,9 +91,11 @@ public async Task StopAsync(CancellationToken cancellationToken)
		_started = false;

Stop Listener when disposed (#104) #105

Stop Listener when disposed (#104) #105

Conversation

alrod commented Feb 3, 2022 • edited Loading

brettsam left a comment

Choose a reason for hiding this comment

sidkri Feb 8, 2022

Choose a reason for hiding this comment

sidkri Feb 8, 2022

Choose a reason for hiding this comment

sidkri Feb 8, 2022

Choose a reason for hiding this comment

sidkri Feb 8, 2022

Choose a reason for hiding this comment

sidkri left a comment

Choose a reason for hiding this comment

sidkri Feb 11, 2022

Choose a reason for hiding this comment

sidkri Feb 11, 2022

Choose a reason for hiding this comment

sidkri Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

sidkri Feb 11, 2022

Choose a reason for hiding this comment

alrod commented Feb 3, 2022 •

edited

Loading

sidkri Feb 11, 2022 •

edited

Loading