[BUG] EventHubListener causes message lost in shutdown about azure-sdk-for-net HOT 11 CLOSED

Thank you for your feedback. Tagging and routing to the team member best able to assist.

from azure-sdk-for-net.

I think this instance also needs updating - https://github.com/JoshLove-msft/azure-sdk-for-net/blob/4a51f3567f5da030a964494807aa25e1aba888aa/sdk/eventhub/Microsoft.Azure.WebJobs.Extensions.EventHubs/src/Listeners/EventHubListener.PartitionProcessor.cs#L289

from azure-sdk-for-net.

@JoshLove-msft: Would you please advise on whether this is related to the ask on the Functions team to expose whether an execution would be retried if the host is not shutting down? If so, please transfer to the Functions host repo.

from azure-sdk-for-net.

@yfujiwara-sansan, I couldn't reproduce having _functionExecutionToken be canceled with linkedCts still not being cancelled. That said, it looks like you are correct that cancellation is not atomic when it comes to linked token sources, so it is possible that one of the sources can be canceled while the linked source is still not canceled. I will make a fix to explicitly check each of the token sources when making the checkpointing decision.

from azure-sdk-for-net.

@JoshLove-msft Thank you for your work! I guess that the repro code sometimes fails to repro due to scheduling of continuations.

By the way, as far as I read #38067 again, it looks that lInkedCts now can be passed to TryExecute bacause it is not lInked to the token which is cancelled In drain mode. I think there is a better option to pass linkedCts to TryExecute, because the app should be cancelled when the load balancer detects ownership lost.

from azure-sdk-for-net.

I think there is a better option to pass linkedCts to TryExecute, because the app should be cancelled when the load balancer detects ownership lost.

The semantics of the token passed to the function are that it is signaled only when shutting down in a way that is not guaranteed to allow the function to complete execution. In terms of whether it makes sense to also cancel this token when partition ownership is lost, I would have to defer to @jsquire on that.

from azure-sdk-for-net.

I understand the semantics of the token passeed to the app. The cancellation behavior for ownership lost should be discussed as a different issue because it is intentional behavior.

Thank you for your answer! I and my colleagues are waiting for fixking the race condition.

from azure-sdk-for-net.

I think there is a better option to pass linkedCts to TryExecute, because the app should be cancelled when the load balancer detects ownership lost.

The semantics of the token passed to the function are that it is signaled only when shutting down in a way that is not guaranteed to allow the function to complete execution. In terms of whether it makes sense to also cancel this token when partition ownership is lost, I would have to defer to @jsquire on that.

@JoshLove-msft : The token that the processor passes to OnProcessingEventBatch will get canceled when partition ownership is lost. I would assume that we're flowing that token into the Executor so that the Function is notified. Looking over the implementation, it seems that we're not passing that along, for some reason. Any idea why?

from azure-sdk-for-net.

I don't think there was a good reason - it may have just been an oversight. Updated the PR to pass linkedCts token to TryExecute.

from azure-sdk-for-net.

@JoshLove-msft

Thank you for fix, but let me confirm just in case.

Is it intentional to fix only single dispatch path? It looks following arguments should be fixed, too:

• https://github.com/JoshLove-msft/azure-sdk-for-net/blob/4a51f3567f5da030a964494807aa25e1aba888aa/sdk/eventhub/Microsoft.Azure.WebJobs.Extensions.EventHubs/src/Listeners/EventHubListener.PartitionProcessor.cs#L176C78-L176C101
• https://github.com/JoshLove-msft/azure-sdk-for-net/blob/4a51f3567f5da030a964494807aa25e1aba888aa/sdk/eventhub/Microsoft.Azure.WebJobs.Extensions.EventHubs/src/Listeners/EventHubListener.PartitionProcessor.cs#L210C63-L210C86

from azure-sdk-for-net.

You are right again. Apologies for the oversight. I will add these updates in.

from azure-sdk-for-net.