fix(core): Return new http client for each httputil call, avoid shared state

[elizabethhealy]

Proposed Changes

• return a new instance of the http client for each httputil call
• avoid shared state between callers

Checklist

• I have added or updated unit tests
• I have added or updated integration tests (if appropriate)
• I have added or updated documentation

Testing Instructions

[gemini-code-assist]

Summary of Changes

Hello @elizabethhealy, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses potential shared state issues within the httputil package by modifying how http.Client instances are provided. Instead of returning a globally shared client, functions now return a new client instance for each call. This change ensures better isolation between HTTP requests and prevents unintended interactions that could arise from multiple callers using the same client instance, thereby improving the robustness and predictability of HTTP operations.

Highlights

• HTTP Client Instantiation: The SafeHTTPClient function now creates and returns a fresh http.Client instance for every invocation, ensuring no shared state between callers.
• Global Client Removal: The previously shared safeHTTPClient global variable has been removed, eliminating a potential source of concurrency issues related to shared client state.
• Consistent Client Provisioning: Functions like SafeHTTPClientWithTLSConfig and SafeHTTPClientWithTransport have been updated to consistently use the new SafeHTTPClient function when a default client is needed, reinforcing the "new instance per call" pattern.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

A client, once shared, Now new with every call, No state to be snared.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly addresses a potential issue with shared mutable state by changing the http.Client singleton to a factory pattern that returns a new client on each call. This is a good improvement. My review includes one suggestion to update the documentation to reflect this new behavior, which will help users of this SDK understand that they receive a new instance each time and can modify it without side effects. This clarification will improve the maintainability and usability of the package.

[github-actions]

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	168.92043ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	97.067828ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	356.774044ms
Throughput	280.29 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	37.551440675s
Average Latency	373.932351ms
Throughput	133.15 requests/second

NANOTDF Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	25.411478145s
Average Latency	253.056165ms
Throughput	196.76 requests/second

[jentfoo]

Can you explain the motivation for this change? Reusing the http.Client is the best way to reuse connections, reducing the latency for ssl handshakes.

It's also considered go best practices to reuse the http.Client.

[elizabethhealy]

Can you explain the motivation for this change? Reusing the http.Client is the best way to reuse connections, reducing the latency for ssl handshakes.

It's also considered go best practices to reuse the http.Client.

@jentfoo im worried about different services/peps calling it to get a client, but one service decides they need a longer timeout so they edit the returned client affecting the rest of the services using it
i assume a specific service/pep would reuse their http client within its setup (hopefully), so it would be on them to ensure theyre reducing that overhead

[jentfoo]

@elizabethhealy Modifying an already in use http.Client would be a concerning pattern, one we should avoid. If custom configurations are needed it's expected that instead a specific client will be constructed with one of the helpers (e.g. SafeHTTPClientWithTransport). Because client reuse is important, and a difficult to communicate burden for external implementors I believe it's easiest if it's done by default. Do you think there are possible API changes we can make while still keeping reuse a default behavior?

If the intention that the implementor handles the de-duplication of the http.Client we would need additional changes to support that. As is this will cause a performance and memory overhead regression.

[jentfoo]

I thought about this API more over night. I think there is the potential for some improvement here. After more thought the fact that empty config is shared and unique configs are not shared could be a source of confusion.

That said, I am still not convinced that anyone is (or should be) modifying the http.Client after returned. And do view this as a simpler option. Let me know your thoughts when you get a chance, or if you want to just put heads together and brainstorm this API some more.

[elizabethhealy]

@jentfoo i agree i think modifying the client after return isnt the right flow a pep should be taking, maybe we opt for more documentation instead of this change to make sure peps dont take that route

[jentfoo]

@elizabethhealy Let me know if you want me to expand the godocs or have other ideas on this API. Client reuse is important for both security and performance, I also care a lot about our API ergonomics to ensure this is easy to understand and adopt.