Skip to content

jsonclient: surface HTTP Do and Read errors #1695

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 1 commit into from
May 6, 2025

Conversation

FiloSottile
Copy link
Contributor

Right now they are shadowed by the Close error.

Note this from the net/http Client.Do docs, which allows simplifying the
logic by far:

On error, any Response can be ignored. A non-nil Response with a
non-nil error only occurs when CheckRedirect fails, and even then the
returned [Response.Body] is already closed.

@FiloSottile FiloSottile requested a review from a team as a code owner May 5, 2025 19:30
@FiloSottile FiloSottile requested review from mhutchinson and removed request for a team May 5, 2025 19:30
@roger2hk
Copy link
Contributor

roger2hk commented May 5, 2025

/gcbrun

Right now they are shadowed by the Close error.

Note this from the net/http Client.Do docs, which allows simplifying the
logic by far:

> On error, any Response can be ignored. A non-nil Response with a
> non-nil error only occurs when CheckRedirect fails, and even then the
> returned [Response.Body] is already closed.
@roger2hk
Copy link
Contributor

roger2hk commented May 5, 2025

/gcbrun

@mhutchinson mhutchinson merged commit bc7acd8 into google:master May 6, 2025
7 checks passed
mcpherrinm added a commit to letsencrypt/boulder that referenced this pull request May 6, 2025
This updates to current `master`, bc7acd89f703743d050f5cd4a3b9746808e0fdae

Notably, it includes a bug-fix to error handling in the HTTP client, which we
found was hiding errors from CT logs, hindering our debugging.

That fix is google/certificate-transparency-go#1695

No release has been tagged since this PR merged, so using the `master` commit.

A few mutual dependencies used by both Boulder and ct-go are updated, including
mysql, otel, and grpc.
aarongable pushed a commit to letsencrypt/boulder that referenced this pull request May 6, 2025
This updates to current `master`,
bc7acd89f703743d050f5cd4a3b9746808e0fdae

Notably, it includes a bug-fix to error handling in the HTTP client,
which we found was hiding errors from CT logs, hindering our debugging.

That fix is
google/certificate-transparency-go#1695

No release has been tagged since this PR merged, so using the `master`
commit.

A few mutual dependencies used by both Boulder and ct-go are updated,
including mysql, otel, and grpc.
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.

3 participants