go-server-timing icon indicating copy to clipboard operation
go-server-timing copied to clipboard

writeHeader can race and be called multiple times

Open bendoerr opened this issue 5 years ago • 1 comments

Because the headerWritten bool is both read and modified outside of the protection of the mutex multiple calls to writeHeader are needlessly made. I suggest moving the headerWritten bool onto the Header type and then inside writeHeader check if it's been set or exit.

bendoerr avatar May 28 '20 23:05 bendoerr

I haven't looked at this code in awhile, but how is headerWritten racy? I don't think the underlying http.ResponseWriter allows for any sort of concurrency, therefore the httpsnoop callbacks can't race either.

Also our second read of headerWritten is after ServeHTTP returns, and the net/http docs on ResponseWriter explicitly state that ResponseWriter calls must not be called after that point, so we can be safe there in reading the value and knowing we can't get a read/write race.

We have to protect writeHeader with a mutex because the actual go-server-timing metrics may be concurrently accessed. Actually, we expect them to be otherwise your server timing is probably just a waterfall. :)

mitchellh avatar May 29 '20 01:05 mitchellh