Repository navigation
Mutex in CaptureMetricsFn #13
Description
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?
Activity
felixge commentedon Mar 1, 2021
IIRC I added this mutex because the application could definitely call
w.Write()on the sameResponseWriterfrom 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 commentedon Mar 1, 2021
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
Writesomehow 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 commentedon Mar 2, 2021
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 commentedon Mar 2, 2021
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.ResponseRecorderdoes nothing to prevent race inWritesuggest me that concurrent Write is not an allowed.7fffffff commentedon Mar 13, 2021
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 commentedon Mar 15, 2021
@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.
Remove mutex and add test for ReaderFrom
Remove mutex and add test for ReaderFrom