Skip to content

Expose a HttpClientHandler property on app config #2883

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

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Mar 25, 2022

Description

This pulls in the GCHandleHolder mechanics from #2777 as I didn't want to block this on the client reset work.

Fixes #2865

TODO

  • Changelog entry
  • Tests (if applicable)

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
nirinchev Nikola Irinchev
@nirinchev nirinchev self-assigned this Mar 25, 2022
@cla-bot cla-bot bot added the cla: yes label Mar 25, 2022
@nirinchev nirinchev requested a review from fealebenpae March 25, 2022 13:36
Copy link
Member

@fealebenpae fealebenpae left a comment

Choose a reason for hiding this comment

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

Looks good, but I have a small default value suggestion.

@@ -150,6 +153,8 @@ public static App Create(AppConfiguration config)
}
}

var httpClient = new HttpClient(config.HttpClientHandler ?? new HttpClientHandler());
Copy link
Member

Choose a reason for hiding this comment

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

I think we should initialize the AppConfiguration.HttpClientHandler property with the default value, instead of leaving it null and instantiating the default handler here.

@coveralls
Copy link

coveralls commented Mar 25, 2022

Pull Request Test Coverage Report for Build 2051898457

  • 20 of 29 (68.97%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 83.239%

Changes Missing Coverage Covered Lines Changed/Added Lines %
Realm/Realm/Native/HttpClientTransport.cs 5 14 35.71%
Files with Coverage Reduction New Missed Lines %
Realm/Realm/Native/HttpClientTransport.cs 1 69.91%
Totals Coverage Status
Change from base Build 2034156449: 0.3%
Covered Lines: 5615
Relevant Lines: 6632

💛 - Coveralls

Verified

This commit was signed with the committer’s verified signature. The key has expired.
nirinchev Nikola Irinchev
@nirinchev nirinchev marked this pull request as ready for review March 28, 2022 11:05

Verified

This commit was signed with the committer’s verified signature.
nirinchev Nikola Irinchev
@nirinchev nirinchev merged commit d0ec4fa into main Mar 28, 2022
@nirinchev nirinchev deleted the ni/custom-http-transport branch March 28, 2022 11:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: CertPathValidatorException on Android 6.
3 participants