Skip to content

Fix #111: Streams now read stderr from started processes

Baptiste Careil requested to merge bcareil/reaction:dev/stderr into main

Here is a first shot at addressing the issue.

I try to add a test but that was a bit messy. I could not add it to a dedicated function because the functions run concurrently and that does not work well neither with the logger (for which there is a potential fix with the crate tracing_test) nor with env::set_current_dir (I tried to work around it using absolute path but hit another roadblock that I did not take the time to figure out).

So, right now, the test in simple.rs is at the end of the only test function to avoid those issues. However, for end-to-end tests, we might want to consider another way to run them. Or to make progress on virtual/modular filters first as this could allow the test framework to run without relying on external commands or the filesystem. For example, if the test framework could have its own action which would be to store matches in a Vec, that would be ideal.

Other than that, Currently, stdout and stderr are polled together and the stream will end when either are closed by the parent process. There might be a case where, when the process exit, we see stderr being closed before processing all messages from stdout that was just flushed.

I can try to handle that case as well. I was thinking of spawning a dedicated task to merge the two Lines into a single mpsc queue. It might make it easier to implement the shutdown logic for #110 (closed) by simplifying the logic in the stream main loop. What do you think ?

Merge request reports

Loading