Fix goroutine leak

[nghialv]

Hi

I know that http.go will be deprecated in the future.
But i have just found a goroutine leak in this file.

A goroutine leak occurs because the goroutine at line 275 never finish when a panic occurred at line 287 .

So i think we should fix it.

[beorn7]

We cannot "fix" it in that way because it would slow down the HTTP processing, ever for those that do not run into the race condition.

We are really in a dead end with the InstrumentHandler function. I'd like to keep it deliberately broken and deprecated rather than "fixing" it, potentially causing even more surprises and harm.

Sorry for that.

[nghialv]

Hi beorn7. Thanks for your comment.

Can you tell me why "it would slow down the HTTP processing"?

[beorn7]

Because you now are doing the length calculation sequentially after serving the HTTP.

It's really waste of our all time to discuss this further. the function is broken and marked as such. I'd rather invest my time in creating new tooling that is not broken by design.

[nghialv]

No, the length calculation is running in a goroutine. (same as the original version) https://github.com/prometheus/client_golang/pull/236/files#diff-b83b53c6fd7712ee815387960471957bR305

But, I'm looking forward to seeing your new design.
Thanks for your reply.

[beorn7]

OK, I see.

But then the header is still ranged over in a goroutine, so you still have a race if the header map is modified. (I read your added comment as "I also fixed the race condition", but that's actually not the case.)

So if I see correctly, the only change here other than moving code around, is to make the channel buffered so that the goroutine calculating the request size, will finish in any case.

I guess one can do that, it's at least an improvement.

[beorn7]

Merging as I got nerd-sniped anyway and invested the effort in reviewing this.

[nghialv]

Thanks beorn7.