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

Implement CloneAsync #447

Closed
tovyhnal opened this issue Apr 23, 2024 · 5 comments
Closed

Implement CloneAsync #447

tovyhnal opened this issue Apr 23, 2024 · 5 comments

Comments

@tovyhnal
Copy link

tovyhnal commented Apr 23, 2024

We are seeing performance degradation of our services (thread processing gets stuck) and after initial investigation one of the suspect is Clone method of ServiceClient.
I checked the implementation of Clone method and there are two things, which can potentially cause performance/stability issues:

  • Sync over async
  • Not accepting/honoring cancellation token and creating new one instead when calling DV

Sync over async
Found at least two places, when we do sync over async during Clone operation, this can block whole thread, which can lead to thread pool starvation.

-WhoAmIResponse resp = Task.Run(async () => await _connectionSvc.GetWhoAmIDetails(this).ConfigureAwait(false)).ConfigureAwait(false).GetAwaiter().GetResult();

-RefreshInstanceDetails(svc, _targetInstanceUriToConnectTo).ConfigureAwait(false).GetAwaiter().GetResult();

Not honoring cancellation token
Also there are places, which don't accept cancellation tokens when calling external service. This can lead to stuck requests and blocking calling thread.
Here is one of multiple examples:
resp = (WhoAmIResponse)(await Command_WebAPIProcess_ExecuteAsync(
req, null, false, null, Guid.Empty, false, _configuration.Value.MaxRetryCount, _configuration.Value.RetryPauseTime, new CancellationToken(), inLoginFlow:true).ConfigureAwait(false));

Would be possible to create CloneAsync(CancellationToken ct), which would be fully async and honor passed cancellation token?

We use v1.1.16

@MattB-msft
Copy link
Member

@tovyhnal

Thanks for the feedback.
In DVSC, you should move away from using Clone + Sync methods to using Async Methods only without clone. Clone + Sync is present for backward compatibility for folks upgrading from the CrmServiceClient.

You will find that the Async methods do not require a cloned connection to scale horizontally.

Regarding cancelation tokens, the tokens are honored only withing the Dataverse service client itself, the API to the server does not currently support them. Once the request is executed on against the server, it cannot be canceled.

Regarding 1.1.16 and clone, We fixed a few issues that were reported with clone in 1.1.17, You can see the full change set here: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/releases/tag/1.1.17
The perf issue your seeing, I believe, we fixed in 1.1.17. ( Fix for Clone being called concurrently causing unnecessary calls to dataverse. GitHub reported - Fix #422)

Please let us know if 1.1.17 solves that for you.. if that does not fix it for you please reopen the issue.
Also, please look at adopting the async methods primarily vs using clone.

@tovyhnal
Copy link
Author

@MattB-msft thanks for reply. I believe the reason, why we Clone the client is described in this issue #93.
When calling DV server, we always use async methods

@MattB-msft
Copy link
Member

@tovyhnal
You should not need to use clone if your using a Singleton. Just do not dispose the client when your done with it.

that said. try it out with the current version of DVSC and see if the perf issue persists.
pretty sure we got that cleaned up.

@mathiasbl
Copy link

mathiasbl commented Apr 26, 2024

@tovyhnal

Thanks for the feedback. In DVSC, you should move away from using Clone + Sync methods to using Async Methods only without clone. Clone + Sync is present for backward compatibility for folks upgrading from the CrmServiceClient.

You will find that the Async methods do not require a cloned connection to scale horizontally.

Regarding cancelation tokens, the tokens are honored only withing the Dataverse service client itself, the API to the server does not currently support them. Once the request is executed on against the server, it cannot be canceled.

Regarding 1.1.16 and clone, We fixed a few issues that were reported with clone in 1.1.17, You can see the full change set here: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/releases/tag/1.1.17 The perf issue your seeing, I believe, we fixed in 1.1.17. ( Fix for Clone being called concurrently causing unnecessary calls to dataverse. GitHub reported - Fix #422)

Please let us know if 1.1.17 solves that for you.. if that does not fix it for you please reopen the issue. Also, please look at adopting the async methods primarily vs using clone.

@MattB-msft Since you are advising to move away from the Sync methods I want to point your attention to the linq provider.
The linq provider (OrgServiceContext) is still very popular to easily write queries. However this is still using the sync methods forcing us to use the Clone method to initialize a context.

Could the Linq provider get some async love?

@MattB-msft
Copy link
Member

@tovyhnal
We would like to do that at some point. However the limitations on that have to do with how Async is currently implemented.
Async support is available only in the client library's right now.. its not available server side yet.

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

No branches or pull requests

3 participants