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

CompositeCloseable: prepend doesn't respect ordering #2097

Merged
merged 2 commits into from
Feb 23, 2022
Merged

CompositeCloseable: prepend doesn't respect ordering #2097

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee514fe
CompositeCloseable: prepend doesn't respect ordering
idelpivnitskiy Feb 18, 2022
13f94cf
Revert default size to 4
idelpivnitskiy Feb 23, 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
9 changes: 4 additions & 5 deletions ...concurrent-api/src/main/java/io/servicetalk/concurrent/api/DefaultCompositeCloseable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.slf4j.LoggerFactory;

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.List;
import java.util.function.Function;
Expand Down Expand Up @@ -175,13 +174,13 @@ private Operand getOrAddOperand(boolean append, boolean isMerge) {

private void appendCloseableDelayError(final AsyncCloseable closeable) {
final Operand operand = getOrAddConcatOperand(true);
operand.closables.add(closeable);
operand.closables.addLast(closeable);
resetState();
}

private void prependCloseableDelayError(final AsyncCloseable closeable) {
final Operand operand = getOrAddConcatOperand(false);
operand.closables.add(closeable);
operand.closables.addFirst(closeable);
resetState();
}

Expand All @@ -203,8 +202,8 @@ private Completable buildCompletable(Function<AsyncCloseable, Completable> close
}

private static final class Operand {
private final List<AsyncCloseable> closables = new ArrayList<>(4);
private final boolean isMerge;
final Deque<AsyncCloseable> closables = new ArrayDeque<>(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

The default size of the collection changes. There is probably no reason not to still use 4 as that still typically fits within the alignment size of a heap allocation of a reference array. ie. The first resize to 4 will probably just reallocate to another array with less wasted space.

final boolean isMerge;

Operand(boolean isMerge) {
this.isMerge = isMerge;
Expand Down
68 changes: 58 additions & 10 deletions ...alk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/CompositeClosableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,21 @@
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.InOrder;

import static io.servicetalk.concurrent.api.AsyncCloseables.newCompositeCloseable;
import static io.servicetalk.concurrent.api.Completable.completed;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

final class CompositeClosableTest {
@ParameterizedTest

@ParameterizedTest(name = "{displayName} [{index}] merge={0} gracefully={1}")
@CsvSource(value = {"true,true", "true,false", "false,true", "false,false"})
void sameOperationDoesNotSOE(boolean merge, boolean gracefully) throws Exception {
AsyncCloseable mockClosable = mock(AsyncCloseable.class);
when(mockClosable.closeAsync()).thenReturn(completed());
when(mockClosable.closeAsyncGracefully()).thenReturn(completed());

AsyncCloseable mockClosable = newMock("asyncCloseable");
CompositeCloseable compositeCloseable = newCompositeCloseable();
for (int i = 0; i < 10000; ++i) {
if (merge) {
Expand All @@ -49,13 +49,10 @@ void sameOperationDoesNotSOE(boolean merge, boolean gracefully) throws Exception
}
}

@ParameterizedTest(name = "gracefully={0}")
@ParameterizedTest(name = "{displayName} [{index}] gracefully={0}")
@ValueSource(booleans = {true, false})
void alternatingOperationSOE(boolean gracefully) {
AsyncCloseable mockClosable = mock(AsyncCloseable.class);
when(mockClosable.closeAsync()).thenReturn(completed());
when(mockClosable.closeAsyncGracefully()).thenReturn(completed());

AsyncCloseable mockClosable = newMock("asyncCloseable");
CompositeCloseable compositeCloseable = newCompositeCloseable();
for (int i = 0; i < 10000; ++i) {
if ((i & 1) == 0) {
Expand All @@ -75,4 +72,55 @@ void alternatingOperationSOE(boolean gracefully) {
}
});
}

@ParameterizedTest(name = "{displayName} [{index}] gracefully={0}")
@ValueSource(booleans = {true, false})
void appendPrependOrder(boolean gracefully) throws Exception {
AsyncCloseable mock1 = newMock("mock1");
AsyncCloseable mock2 = newMock("mock2");
InOrder order = inOrder(mock1, mock2);

CompositeCloseable compositeCloseable = newCompositeCloseable();
compositeCloseable.append(mock2);
compositeCloseable.prepend(mock1);

if (gracefully) {
compositeCloseable.closeGracefully();
order.verify(mock1).closeAsyncGracefully();
order.verify(mock2).closeAsyncGracefully();
} else {
compositeCloseable.close();
order.verify(mock1).closeAsync();
order.verify(mock2).closeAsync();
}
}

@ParameterizedTest(name = "{displayName} [{index}] gracefully={0}")
@ValueSource(booleans = {true, false})
void mergePrependOrder(boolean gracefully) throws Exception {
AsyncCloseable mock1 = newMock("mock1");
AsyncCloseable mock2 = newMock("mock2");
InOrder order = inOrder(mock1, mock2);

CompositeCloseable compositeCloseable = newCompositeCloseable();
compositeCloseable.merge(mock2);
compositeCloseable.prepend(mock1);

if (gracefully) {
compositeCloseable.closeGracefully();
order.verify(mock1).closeAsyncGracefully();
order.verify(mock2).closeAsyncGracefully();
} else {
compositeCloseable.close();
order.verify(mock1).closeAsync();
order.verify(mock2).closeAsync();
}
}

private static AsyncCloseable newMock(String name) {
AsyncCloseable mockClosable = mock(AsyncCloseable.class, name);
when(mockClosable.closeAsync()).thenReturn(completed());
when(mockClosable.closeAsyncGracefully()).thenReturn(completed());
return mockClosable;
}
}