Fix print race in ldmsd_stream_subscribe #1254
Conversation
The msglog printing in ldmsd_stream_subscribe does not lock the print mutex for the entire EVENT printing, leading to interleaving EVENT records in the output. This patch fixed the bug.
|
@narategithub I don't understand this fix. The static variable should be identical to declaring it global. There should only be a single instance of it. There mutex is taken inside the function instead of outside. What am I missing? |
|
I think the purpose of moving the lock to global file scope is so that the lock can be held across multiple calls to msglog(). If the lock is static function scope, the lock can only be held within a single msglog() call. |
| @@ -175,6 +178,7 @@ static int stream_recv_ev(ldms_stream_event_t ev, void *arg) | |||
| if (!events_raw) | |||
| msglog("}"); | |||
| msglog("\n"); | |||
| MSGLOG_UNLOCK(); | |||
There was a problem hiding this comment.
I don't believe it has anything to do with the "scope of the lock" it has to do with the need to hold the lock across multiple calls to msglog(). This is probably what @morrone meant; so it's the description of the problem and the solution that was giving me pause. I was interpreting "scope" to mean "symbol scope."
| @@ -27,18 +27,20 @@ static int daemon_io; | |||
| static int daemon_noroot; | |||
There was a problem hiding this comment.
@narategithub why did you close this?
There was a problem hiding this comment.
#1255 is a simpler fix to this problem.
The msglog printing in ldmsd_stream_subscribe does not lock the print mutex for the entire EVENT printing, leading to interleaving EVENT records in the output. This patch fixed the bug.