Decode resp.Body directly, without ioutil.ReadAll

[dswarbrick]

Since resp.Body implements the io.Reader interface, it can be decoded directly by an xml.Decoder, without the intermediate ioutil.ReadAll byte slice.

[dswarbrick]

Ok, that race was unexpected. http.Client is supposed to be safe for concurrent use by multiple goroutines, but apparently it sometimes isn't. If I use a fresh client in XMLClient.Get instead of the shared one, there is no race.

It appears to only occur when testing the v3.Client due to the Stats function fetching two different URLs, depending on which collector invoked it. It's still not clear why the http client isn't safe for multiple concurrent use though.

[dswarbrick]

I'm pretty sure it's actually a bug with the httptest.Server mock, because adjusting the timeout specified to NewExporter allows the test to succeed.

func (b bindExporterTest) run(t *testing.T) {
	defer b.server.Close()

	o, err := collect(NewExporter(b.version, b.server.URL, 300*time.Millisecond, b.groups))

A timeout of 400ms or greater seems to work.

Of course, this just indicates that the tests are flaky, not the main code.

[dswarbrick]

@SuperQ After letting this stagnate such a long time, would you mind taking a look?

The (previously) failing tests are still a bit baffling tbh. I would not have thought that such a minuscule change could have such a dramatic effect on the tests. Considering that xml.Unmarshal() literally is a one-line wrapper for xml.NewDecoder(r).Decode(v) anyway, the only real difference is that we no longer read the full resp.Body ahead of unmarshalling, and we eliminate a byte slice allocation.

Without wanting to spend too much more time looking at the mock http code, my gut feeling is that they don't handle concurrency very well, and the more efficient unmarshalling is now sufficient to provoke some raciness in the mock functions.