logger icon indicating copy to clipboard operation
logger copied to clipboard

Closed LoggerSet behaves confusingly

Open zudov opened this issue 6 years ago • 1 comments

Of course, LoggerSet isn't supposed to be usable after it was closed (with rmLoggerSet), however it should fail in a way that doesn't cause confusion.

I use this utility to test the current behaviour:

withClosedLogger f = do
  logger <- newStdoutLoggerSet defaultBufSize
  rmLoggerSet logger
  f logger

Trying to flush/close an already closed logger:

> withClosedLogger flushLoggerSet
*** Exception: thread blocked indefinitely in an MVar operation
> withClosedLogger rmLoggerSet
*** Exception: thread blocked indefinitely in an MVar operation

That is if you are lucky. Actually it usually takes it some time blocking after which it throws an exception (or never). It took me some time to figure out that our test suite silently hangs because it tries to close the logger twice.

Trying to write to an already closed logger:

> withClosedLogger (`pushLogStrLn` toLogStr "nope")
>

That is, nothing happens.

Better behavior

To avoid confusion all operations on a closed LoggerSet should be either a no-op (do nothing just return) or an instant, deterministic failure (throwIO LoggerSetIsClosed). I think that:

  • Flushing a closed logger should be a no-op. Conceptually we are asked to flush buffered messages and there's nothing to flush, so we should just return.
  • Closing a closed logger should be a no-op or a failure. Making it an explicit failure would bring a potential lifecycle issue to user's attention. On the other hand it would hardly hurt if the disposable resources are disposed twice. In many cases one could want to do that from several places to "really make sure" that their resources are released. I'd prefer if repeated rmLoggerSet would just silently return
  • Writing to a closed logger should be a failure. If there's a lifecycle problem and something is logged after the logger has been closed it shouldn't silently succeed loosing the important messages. It should throw, so that this problem is discovered and fixed before those messages are actually needed.

zudov avatar Jun 15 '18 16:06 zudov

Uh, just got bitten by this. First time ever hoping for broader linear type adoption.

Abastro avatar May 31 '22 23:05 Abastro