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

net/http: rogue log output to stderr in net/http package #46067

Closed
ross-spencer opened this issue May 9, 2021 · 6 comments
Closed

net/http: rogue log output to stderr in net/http package #46067

ross-spencer opened this issue May 9, 2021 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@ross-spencer
Copy link

ross-spencer commented May 9, 2021

What version of Go are you using (go version)?

$ go version: 1.17 (master/main branch) 

Does this issue reproduce with the latest release?

Yes.

What did you do?

package testpackage

// Run this using $ go test

import (
	"bytes"
	"fmt"
	"io/ioutil"
	"net/http"
	"testing"
)

// RoundTripFuncError describes an interface type that we will then implement.
type RoundTripFuncError func(req *http.Request) *http.Response

// RoundTripError implements Golang's roundtrip interface. In this
// version we want to simulate an error when trying to connect to a
// given SPARQL server.
func (fn RoundTripFuncError) RoundTrip(request *http.Request) (*http.Response, error) {
	return fn(request), fmt.Errorf("Mock error...")
}

// NewTestClientError returns *http.Client with Transport replaced to
// avoid making real calls out to the Internet. Transport will then do
// whatever we request of it. With this version we mock an error in the
// call.
func NewTestClientError(fn RoundTripFuncError) *http.Client {
	return &http.Client{
		Transport: RoundTripFuncError(fn),
	}
}

// TestRoundtrip...
func TestRoundtrip(t *testing.T) {
	httpClient := NewTestClientError(func(req *http.Request) *http.Response {
		return &http.Response{
			StatusCode: 418,
			// Send response to be tested
			Body: ioutil.NopCloser(bytes.NewBufferString("Some response")),
			// Must be set to non-nil value or it panics
			Header: make(http.Header),
		}
	})

	req, err := http.NewRequest("GET", "http://example.com", nil)

	if err != nil {
		// Do something...
	}

	_, err = httpClient.Do(req)
	if err != nil {
		// Do something...
	}
}

What did you expect to see?

Expecting an response or error to handle, or something else? (This looks like a developer's note to self, not necessarily something for everyone?)

What did you see instead?

Seeing an output to stderr which I don't think we are expected to work with. It seems like this would be more idiomatic for the caller to deal with this somewhere, and not have the package itself output anything. I'd be interested to hear more about the intentions of the authorship though.

@ross-spencer ross-spencer changed the title net/http: Rogue log output to stderr in net/http package net/http: rogue log output to stderr in net/http package May 9, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 9, 2021
@seankhliao
Copy link
Member

output is

2021/05/09 17:46:09 RoundTripper returned a response & error; ignoring response

from

log.Printf("RoundTripper returned a response & error; ignoring response")

cc @bradfitz @rsc @empijei

@networkimprov
Copy link

cc @fraenkel @neild

@fraenkel
Copy link
Contributor

The logging is correct. It is letting you know that you violated the RoundTripper contract.

RoundTrip must return err == nil if it obtained
a response, regardless of the response's HTTP status code.
A non-nil err should be reserved for failure to obtain a
response.

ross-spencer added a commit to ross-spencer/spargo that referenced this issue May 17, 2021
ross-spencer added a commit to ross-spencer/spargo that referenced this issue May 17, 2021
@ross-spencer
Copy link
Author

Thanks @fraenkel. That provides helpful advice how to resolve that in my tests. I didn't realize logging was generally recommended in packages. Thanks for the feedback. Should I close this issue?

@fraenkel
Copy link
Contributor

It isn't but there is little else one can do here. Given no error can be returned which would also be rude, a panic is just as bad. The logging was sufficient for you to notice which is its intent.

@bradfitz
Copy link
Contributor

Yeah, I still think logging is the best compromise there. It's helpful but not too intrusive, but enough to get noticed.

@golang golang locked and limited conversation to collaborators May 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants