Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#734: Improve Process Result Model #773

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e25575f
Add single log list
alfeilex Nov 19, 2024
8e66739
Merge branch '#734-improve-process-result' of https://github.com/alfe…
alfeilex Nov 19, 2024
5c3cccb
Merge branch 'devonfw:main' into #734-improve-process-result
alfeilex Nov 19, 2024
4d5a556
Add LogEvent Record
alfeilex Nov 19, 2024
daf4b45
Add LogEvent List to capture Logs
alfeilex Nov 19, 2024
05879d1
Merge branch '#734-improve-process-result' of https://github.com/alfe…
alfeilex Nov 19, 2024
292aa5b
Merge branch 'devonfw:main' into #734-improve-process-result
alfeilex Nov 20, 2024
41a42ce
Merge remote-tracking branch 'origin/main' into #734-improve-process-…
alfeilex Dec 10, 2024
3da75c7
add general List of output messages and rename logevent to outputmess…
alfeilex Dec 10, 2024
540a9b0
Merge branch 'devonfw:main' into #734-improve-process-result
alfeilex Dec 10, 2024
c2b7021
refactor
alfeilex Dec 10, 2024
4530860
refactor processResultImpl in tooldummy
alfeilex Dec 10, 2024
05da168
merge
alfeilex Dec 10, 2024
88c8ef1
change List of Strings for getOutputMessages
alfeilex Dec 10, 2024
4700583
update Test with output message list
alfeilex Dec 11, 2024
f0c5b9e
add test
alfeilex Dec 11, 2024
4daec09
remove forgotten outs in ProcessContextGitMock
alfeilex Dec 11, 2024
4bbb603
add CHANGELOG.adoc
alfeilex Dec 11, 2024
af6add1
Merge branch 'main' into #734-improve-process-result
jan-vcapgemini Dec 12, 2024
8491143
Merge branch 'devonfw:main' into #734-improve-process-result
alfeilex Dec 16, 2024
d7e176e
refactor to lazy getter and remove err and out attributes
alfeilex Dec 17, 2024
ffd6ced
change from synchronized approach to ConcurrentLinkedQueue
alfeilex Dec 17, 2024
4c5f98a
refactor doLog function to output level based stdout and stderr
alfeilex Dec 17, 2024
dce5d5a
add ProcessResult to ProcessContextGitMock and remove redundant
alfeilex Dec 17, 2024
39de3a4
Merge branch 'main' into #734-improve-process-result
alfeilex Dec 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cli/src/main/java/com/devonfw/tools/ide/process/OutputMessage.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.devonfw.tools.ide.process;

/**
* Represent an output message that can be either from stdout or stderr.
*
* @param error A boolean flag that indicates whether the output message is from stdout or stderr
* @param message A string containing the outout message
*/
public record OutputMessage(boolean error, String message) {

}
Original file line number Diff line number Diff line change
Expand Up @@ -168,17 +168,16 @@ public ProcessResult run(ProcessMode processMode) {

this.processBuilder.command(args);

List<String> out = null;
List<String> err = null;
List<OutputMessage> output = new ArrayList<>();

Process process = this.processBuilder.start();

try {
if (processMode == ProcessMode.DEFAULT_CAPTURE) {
CompletableFuture<List<String>> outFut = readInputStream(process.getInputStream());
CompletableFuture<List<String>> errFut = readInputStream(process.getErrorStream());
out = outFut.get();
err = errFut.get();
CompletableFuture<List<String>> outFut = readInputStream(process.getInputStream(), false, output);
CompletableFuture<List<String>> errFut = readInputStream(process.getErrorStream(), true, output);
outFut.get();
errFut.get();
}

int exitCode;
Expand All @@ -189,7 +188,7 @@ public ProcessResult run(ProcessMode processMode) {
exitCode = process.waitFor();
}

ProcessResult result = new ProcessResultImpl(this.executable.getFileName().toString(), command, exitCode, out, err);
ProcessResult result = new ProcessResultImpl(this.executable.getFileName().toString(), command, exitCode, output);

performLogging(result, exitCode, interpreter);

Expand Down Expand Up @@ -220,11 +219,19 @@ public ProcessResult run(ProcessMode processMode) {
* @param is {@link InputStream}.
* @return {@link CompletableFuture}.
*/
private static CompletableFuture<List<String>> readInputStream(InputStream is) {
private static CompletableFuture<List<String>> readInputStream(InputStream is, boolean errorStream, List<OutputMessage> outputMessages) {

return CompletableFuture.supplyAsync(() -> {

try (InputStreamReader isr = new InputStreamReader(is); BufferedReader br = new BufferedReader(isr)) {

String line;
while ((line = br.readLine()) != null) {
synchronized (outputMessages) {
Copy link
Member

@hohwille hohwille Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may create a DeadLock. Java default synchronization is very inefficient and we will have two threads continuously blocking each other.
Consider using a concurrent collection.
I would suggest to use ConcurrentLinkedQueue for collecting the OutputMessages and when completed, convert to a regular List.

Copy link
Member Author

@alfeilex alfeilex Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using ConcurrentLinkedQueue , and at first, it worked fine. However, after running my test case several times, I noticed that the order sometimes changed, causing err1 to appear first in the list of output messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG. Sorry, if I caused a mess here.
Lets have a look at this in January after vacation time.
Maybe we can resolve the problem and make it work deterministic and reliable...
In the worst case, we have to revert the Concurrent-Collection approach and need to go back to your original solution using synchronized (or better using ReentrantLock).

OutputMessage outputMessage = new OutputMessage(errorStream, line);
outputMessages.add(outputMessage);
}
alfeilex marked this conversation as resolved.
Show resolved Hide resolved
}
return br.lines().toList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we try to read the lines again from the buffered reader after it has already been consumed to the end?
IMHO you should change the signature from CompletableFuture<List<String>> to CompletableFuture<Void> and simply return null here.

} catch (Throwable e) {
throw new RuntimeException("There was a problem while executing the program", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ default boolean isSuccessful() {
*/
List<String> getErr();

/**
* @return the {@link List} with {@link OutputMessage} that captured on standard out and standard error lines. Will be {@code null} if not captured but
* redirected.
*/
List<OutputMessage> getOutputMessages();

/**
* Logs output and error messages on the provided log level.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;

import com.devonfw.tools.ide.cli.CliProcessException;
import com.devonfw.tools.ide.context.IdeContext;
Expand All @@ -23,23 +24,25 @@ public class ProcessResultImpl implements ProcessResult {

private final List<String> err;

private final List<OutputMessage> outputMessages;

/**
* The constructor.
*
* @param executable the {@link #getExecutable() executable}.
* @param command the {@link #getCommand() command}.
* @param exitCode the {@link #getExitCode() exit code}.
* @param out the {@link #getOut() out}.
* @param err the {@link #getErr() err}.
* @param outputMessages {@link #getOutputMessages() output Messages}.
*/
public ProcessResultImpl(String executable, String command, int exitCode, List<String> out, List<String> err) {
public ProcessResultImpl(String executable, String command, int exitCode, List<OutputMessage> outputMessages) {

super();
this.executable = executable;
this.command = command;
this.exitCode = exitCode;
this.out = Objects.requireNonNullElse(out, Collections.emptyList());
this.err = Objects.requireNonNullElse(err, Collections.emptyList());
this.outputMessages = Objects.requireNonNullElse(outputMessages, Collections.emptyList());
this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());
this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small simplification:

Suggested change
this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());
this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());
this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).toList();
this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).toList();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
alfeilex marked this conversation as resolved.
Show resolved Hide resolved

@Override
Expand Down Expand Up @@ -72,6 +75,13 @@ public List<String> getErr() {
return this.err;
}

@Override
public List<OutputMessage> getOutputMessages() {

return outputMessages;

}

@Override
public void log(IdeLogLevel level, IdeContext context) {
log(level, context, level);
Expand All @@ -80,11 +90,8 @@ public void log(IdeLogLevel level, IdeContext context) {
@Override
public void log(IdeLogLevel outLevel, IdeContext context, IdeLogLevel errorLevel) {

if (!this.out.isEmpty()) {
doLog(outLevel, this.out, context);
}
if (!this.err.isEmpty()) {
doLog(errorLevel, this.err, context);
if (!this.outputMessages.isEmpty()) {
doLog(outLevel, getOutputMessages().stream().map(OutputMessage::message).collect(Collectors.toList()), context);
}
alfeilex marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;

import com.devonfw.tools.ide.process.OutputMessage;
import com.devonfw.tools.ide.process.ProcessContext;
import com.devonfw.tools.ide.process.ProcessErrorHandling;
import com.devonfw.tools.ide.process.ProcessMode;
Expand All @@ -21,26 +23,26 @@ public class ProcessContextGitMock implements ProcessContext {

private final List<String> arguments;

private final List<String> errors;

private final List<String> outs;
private final List<OutputMessage> outputMessages;

private final LocalDateTime now;

private int exitCode;

private final Path directory;

private final List<String> outs;

/**
* @param directory the {@link Path} to the git repository.
*/
public ProcessContextGitMock(Path directory) {

this.arguments = new ArrayList<>();
this.errors = new ArrayList<>();
this.outs = new ArrayList<>();
this.outputMessages = new ArrayList<>();
this.exitCode = ProcessResult.SUCCESS;
this.directory = directory;
this.outs = new ArrayList<>();
this.now = LocalDateTime.now();
}

Expand All @@ -61,19 +63,30 @@ public void setExitCode(int exitCode) {
}

/**
* @return the {@link List} of mocked error messages.
* @return the {@link List} of mocked stderr messages.
*/
public List<String> getErrors() {

return errors;
return this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. Current code is calling getErrors() or getOuts() in order to add mocked messages to such List.
Streams will create immutable collections that cannot be modified after creation.
In case we leave it like this, also use toList() simplification in all places.

Could you discuss with @jan-vcapgemini who IMHO created this class about the design?
Wouldn't it be smarter to create a regular ProcessResult here and make it accessible so instead of tests calling
processContextGitMock.getErrors().add("fake error message") we could call processContextGitMock.getProcessResult().getOutputMessages().add(new OutputMessage("fake error message", true))
to make it work.
There is a lot of pointless redundancy in this class related to ProcessResultImpl that can be avoided this way.

BTW: I just noticed that you already found this problem and had to update an existing test-case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want a convenience method for adding faked out or err messages, you could still provide them here but delegating to the ProcessResult. Also this way we could even allow a setter for the mocked ProcessResult that also gives us more flexibility (resetting the result for a next faked git call, etc.).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


}

/**
* @return the {@link List} of mocked out messages.
* @return the {@link List} of mocked stdout messages.
*/
public List<String> getOuts() {

return outs;
return this.outputMessages.stream().filter(output -> !output.error()).map(OutputMessage::message).collect(Collectors.toList());

}

/**
* @return the {@link List} of mocked {@link OutputMessage}.
*/
public List<OutputMessage> getOutputMessages() {

return this.outputMessages;

}

@Override
Expand Down Expand Up @@ -139,7 +152,8 @@ public ProcessResult run(ProcessMode processMode) {
// part of git cleanup checks if a new directory 'new-folder' exists
if (this.arguments.contains("ls-files")) {
if (Files.exists(this.directory.resolve("new-folder"))) {
this.outs.add("new-folder");
OutputMessage outputMessage = new OutputMessage(false, "new-folder");
this.outputMessages.add(outputMessage);
this.exitCode = 0;
}
}
Expand Down Expand Up @@ -181,7 +195,7 @@ public ProcessResult run(ProcessMode processMode) {
}
}
this.arguments.clear();
return new ProcessResultImpl("git", command.toString(), this.exitCode, this.outs, this.errors);
return new ProcessResultImpl("git", command.toString(), this.exitCode, this.outputMessages);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.devonfw.tools.ide.context.ProcessContextGitMock;
import com.devonfw.tools.ide.io.FileAccess;
import com.devonfw.tools.ide.io.FileAccessImpl;
import com.devonfw.tools.ide.process.OutputMessage;

/**
* Test of {@link GitContext}.
Expand Down Expand Up @@ -89,7 +90,8 @@ public void testRunGitPullWithoutForce(@TempDir Path tempDir) {
// arrange
String gitRepoUrl = "https://github.com/test";
IdeTestContext context = newGitContext(tempDir);
this.processContext.getOuts().add("test-remote");
OutputMessage outputMessage = new OutputMessage(false, "test-remote");
this.processContext.getOutputMessages().add(outputMessage);
FileAccess fileAccess = new FileAccessImpl(context);
Path gitFolderPath = tempDir.resolve(".git");
fileAccess.mkdirs(gitFolderPath);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,18 @@ public void enablingCaptureShouldRedirectAndCaptureStreamsWithErrorsCorrectly()
assertThat(context).log(IdeLogLevel.ERROR).hasMessageContaining("another error message to stderr");
}

@Test
public void defaultCaptureShouldCaptureStreamsWithCorrectOrder() {
// arrange
IdeTestContext context = newContext(PROJECT_BASIC, null, false);

// act
context.newProcess().executable(TEST_RESOURCES.resolve("process-context").resolve("log-order.sh")).run();

// assert
assertThat(context).log(IdeLogLevel.INFO).hasEntries("out1", "err1", "out2", "err2");
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After several runs, you will see that the order is not deterministic, probably because the choice of the first stdout or stderr thread is not fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If synchronized is to inefficent and we stick to ConcurrentLinkedQueue then we have to accept the compromise that the order is not 100% guaranteed. Also, the test-case doesn't work and testing the order like in the implemented test-case isn't possible and has to be removed before merging

private IdeLogLevel convertToIdeLogLevel(ProcessErrorHandling processErrorHandling) {

return switch (processErrorHandling) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public ProcessResult runTool(ProcessMode processMode, GenericVersionRange toolVe

// skip installation but trigger postInstall to test mocked plugin installation
postInstall(true);
return new ProcessResultImpl(this.tool, this.tool, 0, List.of(), List.of());
return new ProcessResultImpl(this.tool, this.tool, 0, List.of());
}

@Override
Expand Down
6 changes: 6 additions & 0 deletions cli/src/test/resources/process-context/log-order.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/bash
echo "out1"
echo "err1" >&2
sleep 5
echo "out2"
echo "err2" >&2
Loading