Use pipes instead of file for logging in tests #3074
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zeegomo
force-pushed
the
logger-from-pipe
branch
from
120f300 to
64e94f3
Compare
|
Does this replace #3051 as a proper fix to the problem? |
|
Yes, this should solve that problem |
|
Please close #3051 then. |
|
This is ready for review, it is still a draft because it points to a patched jortestkit |
|
input-output-hk/jortestkit#59 merged, please update and rebase. |
mzabaluev
reviewed
Mar 3, 2021
| log_location: PathBuf, | ||
| error_lines: String, | ||
| }, | ||
| #[error("error in logs. Error lines: {error_lines}, full content:{logs}")] |
There was a problem hiding this comment.
Suggested change
| #[error("error in logs. Error lines: {error_lines}, full content:{logs}")] | |
| #[error("error in logs. Error lines: {error_lines}, full content: {logs}")] |
Not really sure it's useful to put the full JSON content in a single error printout. Maybe the test fixture handling this error can re-parse the logs field and print the lines out nicely?
There was a problem hiding this comment.
Maybe we could reuse log formatting from tracing_subscriber::fmt but I'm not sure if it can be used independently, need to check.
zeegomo
force-pushed
the
logger-from-pipe
branch
from
210e52e to
993ec4e
Compare
mzabaluev
force-pushed
the
logger-from-pipe
branch
from
993ec4e to
ee7bf92
Compare
|
The nightly failure should be fixed by #2888 |
mzabaluev
reviewed
Mar 4, 2021
mzabaluev
approved these changes
Mar 4, 2021
Reading from file has some challenges associated with it. Namely on some Os reads can see torn writes and invalid utf-8 sequences. In addition, it is also pretty inefficient to read from file every time we want to access the logs. This solution is not the best one, we probably want to use some other library or method that allows us to read from stdout in a non blocking way.
Trait objects impose unnecessary bound for Sync/Send, so let's use generics instead. Add a dummy implementer of SyncNode to make using those types easier and almost identical as before.
Move selection of node inside the method to avoid having to clone the nodes. It would be impossible to use a reference since the argument must be a immutable reference but the method needs a mutable one, and they would be both from the same struct.
Change 'controller' method to consume self and add logger, since we do not read from file anymore. Client can then use a reference to the logger, but is important that there is only one logger that reads from the child stdout.
With the new architecture there is only one instace of logger per process, and this instance is controlled by the process itself. It also makes sense that the process is responsible for knowing when it started properly.
zeegomo
force-pushed
the
logger-from-pipe
branch
from
13efbc8 to
483f1ae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use a pipe for reading logs from child processes instead of reading from files, because that caused synchronization problems.
The main problem is that reading from stdout is blocking, the current solution is to spawn a new thread that blocks on new messages and then send them back on a channel. It's not ideal because we spawn a new thread for each new process, but it should not be too much of a problem.
I also tried with async processes and output (
tokio::process) but I had issues with I/O drivers not receiving updates in some cases and I dropped it.Another option could be to use some crate that exposes non future-based non blocking reads (mio only supports unix)