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

fix(client): Prevent disposing of the HttpMessageHandler in CamundaCoudTokenProvider #222

Closed
wants to merge 1 commit into from

Conversation

terjeinnerdal
Copy link
Contributor

Creates the HttpClient in CamundaCloudTokenProvider constructor. Added CamundaCloudTokenProvider.SetHttpMessageHandler method to set the HttpMessageHandler for testing which will create a new HttpClient.

closes #218

@ChrisKujawa
Copy link
Collaborator

Hey @terjeinnerdal thanks for fixing this. Could we add a separate test which fails before and now succeeds with the change?

@terjeinnerdal
Copy link
Contributor Author

Yes, that sounds like a good idea. I'll see if I can find the time today.

…oudTokenProvider

Includes a test which ensures ObjectDisposedException is not thrown when renewing the access token, which will fail if the fix is reverted/removed.
@terjeinnerdal
Copy link
Contributor Author

I've included a test now, but the continuous-integration check fails on the integration tests. It shows the following warning:
"toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
TearDown failed for test fixture Client.IntegrationTests.WorkflowInstanceTest
Docker.DotNet.DockerContainerNotFoundException : Docker API responded with status code=NotFound, response={"message":"No such image: camunda/zeebe:0.23.0"}"

@ChrisKujawa ChrisKujawa self-requested a review February 25, 2021 06:49
Copy link
Collaborator

@ChrisKujawa ChrisKujawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and the additional test 👍 I think we can simplify the test and then we can merge it :)

{
// given
ExpiresIn = 0;
var firstToken = await TokenProvider.GetAccessTokenForRequestAsync();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var firstToken = await TokenProvider.GetAccessTokenForRequestAsync();
await TokenProvider.GetAccessTokenForRequestAsync();

Comment on lines +248 to +250
var files = Directory.GetFiles(TokenStoragePath);
var tokenFile = files[0];
await File.WriteAllTextAsync(tokenFile, "FILE_TOKEN");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var files = Directory.GetFiles(TokenStoragePath);
var tokenFile = files[0];
await File.WriteAllTextAsync(tokenFile, "FILE_TOKEN");

await File.WriteAllTextAsync(tokenFile, "FILE_TOKEN");

// when
Token = "SECOND_TOKEN";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Token = "SECOND_TOKEN";

@ChrisKujawa
Copy link
Collaborator

Thank you very much for your contribution @terjeinnerdal I created a separate PR to do the clean up for you and to merge it with the latest changes. #235

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

Successfully merging this pull request may close these issues.

HttpMessageHandler is disposed in CamundaCloudTokenProvider
2 participants