router icon indicating copy to clipboard operation
router copied to clipboard

investigate standard IO locking

Open Geal opened this issue 1 year ago • 3 comments

when writing logs to stdout and stderr, we use the standard library's StdOut, which requires locking to make sure multiple threads don't try to write lines at the same time.

There's a performance issue in there, but also, there's the potential to deadlock the router. (I am not entirely clear on the details here) if for some reason one thread takes the stdout lock and does not release it, every other thread that would try to write a log line at that moment would be blocked too, and then the router would be unable to process any more requests.

It should be possible to take the lock once on startup, then keep it the entire time the format layer is using it. But then, what happens on reloads?

Geal avatar Feb 07 '24 16:02 Geal

maybe we should look at https://docs.rs/tracing-appender/latest/tracing_appender/ ?

Geal avatar Feb 08 '24 09:02 Geal

the idea of running a separate thread to handle logs does not sit well with me right now, but I'll give it a try.

The alternative would be to skip stdout's lock/LineWriter/BufWriter pipeline (because it blocks on flushing the lines) and write directly to stdout:

  • create a File from the stdout fd (with a special case for windows, or accepting that the bug would still happen on windows)
  • make this fd non blocking
  • add back a LlineWriter/BufWriter alternative that supports a kind of try_write option

making stdout non blocking has one risk: any use of println! can now panic

Geal avatar Feb 08 '24 17:02 Geal

I tested with tracing_appender, and it is still blocking. To test a clogged stdout, I create a pipe with mkfifo, start the router with --log=trace and redirect the router's output to that pipe, and on the other end, read from the pipe with less -f, but staying on the first page. Then in another shell, I launch router queries. After a few, a router query will just never answer, with the current router from dev, with or without tracing_appender

Geal avatar Feb 09 '24 13:02 Geal