itamarst / eliot

Eliot: the logging system that tells you *why* it happened
https://eliot.readthedocs.io
Apache License 2.0
1.1k stars 66 forks source link

Clarify the thread-safety guarantees of ILogger #382

Closed exarkun closed 5 years ago

exarkun commented 5 years ago

ILogger has nothing to say about thread-safety. Action claims that an individual instance must only be used in a single thread which strongly implies that using Action in different threads concurrently (so long as the instance constraint is obeyed) is allowed and supported. This in turn seems to imply that ILogger must be thread-safe since all Actions use the global ILogger provider by default.

The current implementation of Logger seems to be mostly thread-safe so long as Destinations.send is thread-safe. I wouldn't swear that Logger.write is safe for concurrent use with the other Destination APIs (eg add_destinations). That might be something to verify/fix/document but it doesn't seem like a major problem if those operations aren't thread-safe with respect to each other since add_destinations and the other similar APIs are mostly an initialization-time thing. What's really important is that Logger.write be safe to call from multiple threads concurrently. And it does seem to be.

But the other implementation of ILogger, MemoryLogger, is totally not thread safe. If ILogger.write is meant to be thread-safe ("if Eliot is meant to be usable in multithreaded programs") then MemoryLogger should be fixed to also provide this guarantee, otherwise testing Eliot-using code randomly fails - and does so in ways that real use of Eliot likely shouldn't and wouldn't.

itamarst commented 5 years ago

I think correct answer here is "make MemoryLogger thread-safe", yeah.

exarkun commented 5 years ago

Trying out a very naive approach to MemoryLogger thread-safety: https://github.com/itamarst/eliot/tree/382.threadsafe-memorylogger