Mutex in CaptureMetricsFn

[ebati]

I opened an issue in opentelemetry-go-contrib about using this library.

I will send a PR to add another hook for ReadFrom but i am not sure why the mutex in your implementation of CaptureMetricsFn is used. Is concurrent call to those functions possible or is there any reason to use mutex in this context?

[felixge]

IIRC I added this mutex because the application could definitely call w.Write() on the same ResponseWriter from two goroutines. Arguably that wouldn't be a good idea, and maybe it shouldn't be the job of my library to deal with such pathological application behavior. What do you think?

[ebati]

I think the same that concurrent write calls are not a normal use.

I just have a hesitation even if the order of written data is incorrect, one might implement some kind of packet data and write packets concurrently. If a call to Write somehow writes whole data to the underlying connection at once and ensures this property (I couldn't see anything in the documentation), that might be a valid edge case. For example Logger has this kind of guarantee.

[felixge]

I'm not sure I completely follow your last comment.

Anyway, the way I look at it is: In the worst case the mutex is redundant overhead. In the best case it will prevent bugs. Precautionary principle slightly guides me towards keeping it. But I could be convinced otherwise if somebody wants to do some more research.

Is the presence of the mutex causing you any issues for now?

[ebati]

No, there is not any issue for now. I just want to learn why mutex is used.

I didn't checked the http code yet but httptest.ResponseRecorder does nothing to prevent race in Write suggest me that concurrent Write is not an allowed.

[7fffffff]

I think ebati was pointing out that if an application has multiple goroutines writing to the same Writer, a mutex is enough to ensure the correct ordering when each Write is a complete unit of data, like a log line. If the goroutines Write partial data, only the application knows how those goroutines need to be synchronized and the mutex does not add safety.

I'm inclined to say that it's the application's responsibility to serialize access to a ResponseWriter, so the mutex is unnecessary.

[felixge]

@ebati @7fffffff ok, I think y'all convinced me : ). If any of you wants to send a PR to remove the mutex, I'd be happy to merge it.