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.

Sign up for GitHub

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

Jump to bottom

After surefire 2058 #519

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

After surefire 2058 #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3363a1c
Commits after SUREFIRE-2058 regarding AbstractStreamDecoder
Tibor17 Apr 12, 2022
72b1841
Refactored decode(Memento) and embedded Memento in the decoder instance
Tibor17 Apr 13, 2022
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
4 changes: 1 addition & 3 deletions ...common/src/main/java/org/apache/maven/plugin/surefire/extensions/EventConsumerThread.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import org.apache.maven.surefire.api.event.Event;
import org.apache.maven.surefire.api.fork.ForkNodeArguments;
import org.apache.maven.surefire.api.stream.AbstractStreamDecoder.Memento;
import org.apache.maven.surefire.extensions.CloseableDaemonThread;
import org.apache.maven.surefire.extensions.EventHandler;
import org.apache.maven.surefire.extensions.util.CountdownCloseable;
Expand Down Expand Up @@ -67,10 +66,9 @@ public void run()
CountdownCloseable c = countdownCloseable;
EventDecoder eventDecoder = decoder )
{
Memento memento = eventDecoder.new Memento();
do
{
Event event = eventDecoder.decode( memento );
Event event = eventDecoder.decode();
if ( event != null && !disabled )
{
eventHandler.handleEvent( event );
Expand Down
11 changes: 10 additions & 1 deletion maven-surefire-common/src/main/java/org/apache/maven/surefire/stream/EventDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ public class EventDecoder extends AbstractStreamDecoder<Event, ForkedProcessEven

private final OutputStream debugSink;

private Memento memento;

public EventDecoder( @Nonnull ReadableByteChannel channel,
@Nonnull ForkNodeArguments arguments )
{
Expand All @@ -148,8 +150,15 @@ public EventDecoder( @Nonnull ReadableByteChannel channel,
}

@Override
public Event decode( @Nonnull Memento memento ) throws IOException
public Event decode() throws IOException
{
if ( memento == null )
{
// do not create memento in constructor because the constructor is called in another thread
// memento is the thread confinement object
memento = new Memento();
}

try
{
ForkedProcessEventType eventType = readMessageType( memento );
Expand Down
43 changes: 20 additions & 23 deletions surefire-api/src/main/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,15 @@ protected AbstractStreamDecoder( @Nonnull ReadableByteChannel channel,
logger = arguments.getConsoleLogger();
}

public abstract M decode( @Nonnull Memento memento ) throws MalformedChannelException, IOException;
/**
* Decoding and returns a message {@code M} and waiting, if necessary, for the next
* message received from the channel.
*
* @return message {@code M}, or null if could not decode a message due to a frame error
* @throws MalformedChannelException the channel error
* @throws IOException stream I/O exception
*/
public abstract M decode() throws MalformedChannelException, IOException;

@Nonnull
protected abstract byte[] getEncodedMagicNumber();
Expand Down Expand Up @@ -305,37 +313,26 @@ private String readString( @Nonnull final Memento memento, @Nonnegative final in
memento.getDecoder().reset();
final CharBuffer output = memento.getCharBuffer();
( (Buffer) output ).clear();
final ByteBuffer input = memento.getByteBuffer();
final List<String> strings = new ArrayList<>();
int countDecodedBytes = 0;
for ( boolean endOfInput = false; !endOfInput; )
{
final int bytesToRead = totalBytes - countDecodedBytes;
final int bytesToRead = totalBytes - countDecodedBytes; // our wish to read bytes as much as possible
read( memento, bytesToRead );
int bytesToDecode = min( input.remaining(), bytesToRead );
final ByteBuffer input = memento.getByteBuffer();
int bytesToDecode = min( input.remaining(), bytesToRead ); // our guarantee of available bytes in buffer
final boolean isLastChunk = bytesToDecode == bytesToRead;
endOfInput = countDecodedBytes + bytesToDecode >= totalBytes;
do
{
boolean endOfChunk = output.remaining() >= bytesToRead;
boolean endOfOutput = isLastChunk && endOfChunk;
int readInputBytes = decodeString( memento.getDecoder(), input, output, bytesToDecode, endOfOutput,
memento.getLine().getPositionByteBuffer() );
bytesToDecode -= readInputBytes;
countDecodedBytes += readInputBytes;
}
while ( isLastChunk && bytesToDecode > 0 && output.hasRemaining() );

if ( isLastChunk || !output.hasRemaining() )
{
strings.add( ( (Buffer) output ).flip().toString() );
( (Buffer) output ).clear();
}
boolean endOfChunk = output.remaining() >= bytesToRead;
boolean endOfOutput = isLastChunk && endOfChunk;
int readInputBytes = decodeString( memento.getDecoder(), input, output, bytesToDecode, endOfOutput,
memento.getLine().getPositionByteBuffer() );
countDecodedBytes += readInputBytes;
strings.add( ( (Buffer) output ).flip().toString() );
( (Buffer) output ).clear();
memento.getLine().setPositionByteBuffer( 0 );
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason of resetting line position here?

Will give this branch a try later. Maybe compare events dump before and after on single test runs, and just see if larger projects have no other issues.

I think I already tried with removed do-while once, because it was suspicious at first look. But didn't want to change anything else in #518 , but my guess was that that wile condition is never true.

Copy link
Contributor Author

@Tibor17 Tibor17 Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@zoltanmeze memento.getLine() holds the line which could not be properly decoded and it is eligible for printing a corruption status on the console.

Copy link
Contributor Author

@Tibor17 Tibor17 Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After properly decoded bytes, we clear the buffer and new bytes will be decoded. The corrupted line has to reset to 0 and remaining is 0 too of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If MalformedFrameException is caught, whole byte sequence is printed as a corrupted string starting by memento.getLine().getPositionByteBuffer() up to the buffer's position.

Copy link
Contributor

@zoltanmeze zoltanmeze Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Wouldn't read( memento, bytesToRead ); also reset this to 0 in every iteration starting from second?

Copy link
Contributor Author

@Tibor17 Tibor17 Apr 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The memento holds the reference to the buffer too. And if I clean it up, I should reset the position variable via memento.getLine().setPositionByteBuffer( 0 ) in order to be consistent.

}

memento.getDecoder().reset();
( (Buffer) output ).clear();

return toString( strings );
}

Expand Down
2 changes: 1 addition & 1 deletion ...ire-api/src/main/java/org/apache/maven/surefire/api/stream/MalformedChannelException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/

/**
*
* No supported message type.
*/
public class MalformedChannelException extends Exception
{
Expand Down
2 changes: 1 addition & 1 deletion ...ire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ protected Mock( @Nonnull ReadableByteChannel channel, @Nonnull ForkNodeArguments
}

@Override
public Event decode( @Nonnull Memento memento ) throws MalformedChannelException
public Event decode() throws MalformedChannelException
{
throw new MalformedChannelException();
}
Expand Down
11 changes: 1 addition & 10 deletions ...fire-booter/src/main/java/org/apache/maven/surefire/booter/spi/CommandChannelDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.apache.maven.surefire.api.booter.Command;
import org.apache.maven.surefire.api.booter.MasterProcessChannelDecoder;
import org.apache.maven.surefire.api.fork.ForkNodeArguments;
import org.apache.maven.surefire.api.stream.AbstractStreamDecoder.Memento;
import org.apache.maven.surefire.api.stream.MalformedChannelException;
import org.apache.maven.surefire.booter.stream.CommandDecoder;

Expand All @@ -40,7 +39,6 @@
public class CommandChannelDecoder implements MasterProcessChannelDecoder
{
private final CommandDecoder decoder;
private Memento memento;

public CommandChannelDecoder( @Nonnull ReadableByteChannel channel,
@Nonnull ForkNodeArguments arguments )
Expand All @@ -53,18 +51,11 @@ public CommandChannelDecoder( @Nonnull ReadableByteChannel channel,
@SuppressWarnings( "checkstyle:innerassignment" )
public Command decode() throws IOException
{
if ( memento == null )
{
// do not create memento in constructor because the constructor is called in another thread
// memento is the thread confinement object
memento = decoder.new Memento();
}

do
{
try
{
Command command = decoder.decode( memento );
Command command = decoder.decode();
if ( command != null )
{
return command;
Expand Down
10 changes: 9 additions & 1 deletion surefire-booter/src/main/java/org/apache/maven/surefire/booter/stream/CommandDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public class CommandDecoder extends AbstractStreamDecoder<Command, MasterProcess

private final ForkNodeArguments arguments;
private final OutputStream debugSink;
private Memento memento;

public CommandDecoder( @Nonnull ReadableByteChannel channel,
@Nonnull ForkNodeArguments arguments )
Expand All @@ -80,8 +81,15 @@ public CommandDecoder( @Nonnull ReadableByteChannel channel,
}

@Override
public Command decode( @Nonnull Memento memento ) throws IOException, MalformedChannelException
public Command decode() throws IOException, MalformedChannelException
{
if ( memento == null )
{
// do not create memento in constructor because the constructor is called in another thread
// memento is the thread confinement object
memento = new Memento();
}

try
{
MasterProcessCommand commandType = readMessageType( memento );
Expand Down