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

.NET MFD throw exception in EnsureCompleted when run test sync API with debug version SDK #4702

Open
Tracked by #4490
chunyu3 opened this issue May 13, 2024 · 4 comments
Assignees
Labels
DPG/RLC v2.1 Post Gallium work DPG

Comments

@chunyu3
Copy link
Member

chunyu3 commented May 13, 2024

When build SDK in Debug version, and run sync API of MFD operation, exception throw Task is not completed in EnsureCompleted

image

@chunyu3
Copy link
Member Author

chunyu3 commented May 20, 2024

In MultipartFormDataRequestContent.WriteTo function, it will call _multipartContent.CopyToAsync(stream).EnsureCompleted(); for .NET which is not NET6, such as Net standard2.0. But in EnsureCompleted function, there is an assert to check if the task is completed (see following). So when we run sync method with a DEBUG version SDK, it will fail.

MultipartFormDataRequestContent.WriteTo
image

TaskExtensions.EnsureCompleted
image

@chunyu3
Copy link
Member Author

chunyu3 commented May 20, 2024

Possible solution:
We will not use TaskExtensions.EnsureCompleted function for azure SDK, and directly call _multipartContent.CopyToAsync(stream).GetAwaiter().GetResult(); as what we do for non-azure SDK.

@annelo-msft
Copy link
Member

@chunyu3, could you please share a repro case for this issue?

@annelo-msft
Copy link
Member

annelo-msft commented May 21, 2024

Per offline discussion, it is by-design that we are doing sync-over-async in the sync/netstandard2.0 case, and cannot be avoided when using the BCL MultipartFormDataContent type. Given this, the recommendation is:

  1. Add a net6.0 target to libraries that send multipart/form-data requests. This would look like the following change to the .csproj file:
-<TargetFrameworks>$(RequiredTargetFrameworks)</TargetFrameworks>
+<TargetFrameworks>$(RequiredTargetFrameworks);net6.0</TargetFrameworks>
  1. Change the internally emitted class to call GetAwaiter().GetResult() instead of EnsureCompleted(), (e.g. here). This would look like:

image

The Core team has work in progress to add the net6.0 TFM as a RequiredTargetFrameworks for all client libraries. However, this requires further due-diligence and investigation, so it is important that this change not yet be added to all client library .csproj files -- only those using multipart/form-data.

Please note that if we do only item 2 without also doing item 1, clients will do sync-over-async even on net6.0 platforms and this is highly undesirable. Please only do the two together. This relates to the guidance that we should not include the emitted MultipartFormDataRequestContent.cs in clients that do not use it.

Also, because we will be doing sync-over-async in these cases, there is a chance that these tests may fail nondeterministically. If they become flaky, we should find a way to [Ignore] them, but only for the MPFD/netstandard2.0/sync cases -- it would be preferable to continue testing the net6.0+/sync cases in case we identify further issues that need to be investigated and corrected.

@jsquire FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DPG/RLC v2.1 Post Gallium work DPG
Projects
None yet
Development

No branches or pull requests

2 participants