commercialhaskell / rio

A standard library for Haskell
Other
836 stars 54 forks source link

logOptionsHandle -> logSend is not thread-safe #231

Closed K0Te closed 3 years ago

K0Te commented 3 years ago

Hi,

I am using RIO as a base for an application, which combines web server and event-sourcing handler for tasks.

-- | Run all microservices forever, terminate everything on exception
run :: RIO App ()
run = foldr concurrently_ WebHandler.main
    [ WebParser.main
    ,Service1.DeploymentService.main
    , Service2.DeploymentService.main
    ]

Logging setup:

runApp::  RIO App a -> IO a
runApp app  = do
  lo <- logOptionsHandle stderr True
  withLogFunc lo $ \lf ->
    let ... 
     in runRIO appConfig app

As a result, multiple services logging gets combined into a single output stream, looks like the default logSend does not have any thread-safety protection.

snoyberg commented 3 years ago

Can you provide a minimal repro? And do you know if you're logging in UTF-8? It's known to potentially interleave output for non-UTF-8 output.

K0Te commented 3 years ago

I've been logging to stderr in ubuntu:18.04 docker image. And it turn out that it stderr is non-UTF-8 be default:

Prelude GHC.IO.Handle.Internals GHC.IO.Handle.Types GHC.IO.Handle System.IO> wantWritableHandle "a" stderr (\x -> pure $ haCodec x)
Just ASCII
root@3b1b7a80967f:/# LANG=C.UTF-8  /usr/local/bin/stack ghci
Prelude System.IO GHC.IO.Handle GHC.IO.Handle.Internals GHC.IO.Handle.Types> wantWritableHandle "a" stderr (\x -> pure $ haCodec x)
Just UTF-8

This explains why everything worked fine on my Mac, but failed after deploying to K8. Thank you for a prompt reply!

Is this anything that should be fixed, or just behavior by design?

snoyberg commented 3 years ago

It's definitely "by design" in this case: the text package unfortunately does I/O very poorly, and we're relying on its behavior for non-UTF-8 backends. We could theoretically do some locking, but that will only fix the issue within rio; other pieces of the system still working directly with stderr could have issues. Overall, my preference is to fix this with documentation, and advertise loudly that non-interleaving is only accounted for with UTF-8 output. Would you be open to writing a PR with such a doc fix?