Skip to content

Commit 8ac5599

Browse files
authored
servlet: fix threadingTest and update lincheck (#12306)
Seems like previously it was not testing all the flows but only: the write/flush calls are added to the queue by `runOrBuffer` because `readyAndDrained` is initially `false`. So, * `isReady()` is never called * `isReadyReturnedFalse` never set to true * `maybeOnWritePossible()` does nothing All that makes me think that #9917 is caused by some bugs in older versions of llincheck, can be closed now. A more real simulation happens if calling `onWritePossible` first via `initialOnWritePossible`, and then it has found a race condition in `AsyncServletOutputStreamWriterConcurrencyTest.isReady()`, if `maybeOnWritePossible()` is executed before `return isReady`. Additionally updated lincheck, * it does not need jvmArgs now * is compatible with java 8 again * changed asserts from Truth to junit, as lincheck was trying to use `com.google.common.truth.Subject.equals(Object)` It still does not detect #12268, but might be needs more time.
1 parent 2ba5c3d commit 8ac5599

File tree

4 files changed

+43
-41
lines changed

4 files changed

+43
-41
lines changed

build.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ subprojects {
238238

239239
// At a test failure, log the stack trace to the console so that we don't
240240
// have to open the HTML in a browser.
241-
tasks.named("test").configure {
241+
tasks.withType(Test).configureEach {
242242
testLogging {
243243
exceptionFormat = 'full'
244244
showExceptions = true

gradle/libs.versions.toml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,7 @@ jetty-servlet = "org.eclipse.jetty.ee10:jetty-ee10-servlet:12.0.16"
6969
jetty-servlet10 = "org.eclipse.jetty:jetty-servlet:10.0.20"
7070
jsr305 = "com.google.code.findbugs:jsr305:3.0.2"
7171
junit = "junit:junit:4.13.2"
72-
# 2.17+ require Java 11+ (not mentioned in release notes)
73-
lincheck = "org.jetbrains.kotlinx:lincheck-jvm:2.16"
72+
lincheck = "org.jetbrains.lincheck:lincheck:3.2"
7473
# Update notes / 2023-07-19 sergiitk:
7574
# Couldn't update to 5.4.0, updated to the last in 4.x line. Version 5.x breaks some tests.
7675
# Error log: https://github.com/grpc/grpc-java/pull/10359#issuecomment-1632834435

servlet/build.gradle

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ dependencies {
4343
testImplementation libraries.javax.servlet.api
4444

4545
threadingTestImplementation project(':grpc-servlet'),
46-
libraries.truth,
46+
libraries.junit,
4747
libraries.javax.servlet.api,
4848
libraries.lincheck
4949

@@ -69,19 +69,12 @@ dependencies {
6969
libraries.protobuf.java
7070
}
7171

72-
tasks.named("test").configure {
73-
if (JavaVersion.current().isJava9Compatible()) {
74-
jvmArgs += [
75-
// required for Lincheck
76-
'--add-opens=java.base/jdk.internal.misc=ALL-UNNAMED',
77-
'--add-exports=java.base/jdk.internal.util=ALL-UNNAMED',
78-
]
79-
}
80-
}
81-
8272
tasks.register('threadingTest', Test) {
8373
classpath = sourceSets.threadingTest.runtimeClasspath
8474
testClassesDirs = sourceSets.threadingTest.output.classesDirs
75+
jacoco {
76+
enabled = false
77+
}
8578
}
8679

8780
tasks.named("assemble").configure {

servlet/src/threadingTest/java/io/grpc/servlet/AsyncServletOutputStreamWriterConcurrencyTest.java

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,23 @@
1616

1717
package io.grpc.servlet;
1818

19-
import static com.google.common.truth.Truth.assertWithMessage;
20-
import static org.jetbrains.kotlinx.lincheck.strategy.managed.ManagedStrategyGuaranteeKt.forClasses;
19+
import static org.jetbrains.lincheck.datastructures.ManagedStrategyGuaranteeKt.forClasses;
20+
import static org.junit.Assert.assertEquals;
21+
import static org.junit.Assert.assertFalse;
22+
import static org.junit.Assert.assertTrue;
2123

2224
import io.grpc.servlet.AsyncServletOutputStreamWriter.ActionItem;
2325
import io.grpc.servlet.AsyncServletOutputStreamWriter.Log;
2426
import java.io.IOException;
2527
import java.util.concurrent.ConcurrentLinkedQueue;
28+
import java.util.concurrent.atomic.AtomicBoolean;
2629
import java.util.concurrent.atomic.AtomicReference;
2730
import java.util.function.BiFunction;
28-
import org.jetbrains.kotlinx.lincheck.LinChecker;
29-
import org.jetbrains.kotlinx.lincheck.annotations.OpGroupConfig;
30-
import org.jetbrains.kotlinx.lincheck.annotations.Operation;
31-
import org.jetbrains.kotlinx.lincheck.annotations.Param;
32-
import org.jetbrains.kotlinx.lincheck.paramgen.BooleanGen;
3331
import org.jetbrains.kotlinx.lincheck.strategy.managed.modelchecking.ModelCheckingCTest;
34-
import org.jetbrains.kotlinx.lincheck.strategy.managed.modelchecking.ModelCheckingOptions;
35-
import org.jetbrains.kotlinx.lincheck.verifier.VerifierState;
32+
import org.jetbrains.lincheck.datastructures.BooleanGen;
33+
import org.jetbrains.lincheck.datastructures.ModelCheckingOptions;
34+
import org.jetbrains.lincheck.datastructures.Operation;
35+
import org.jetbrains.lincheck.datastructures.Param;
3636
import org.junit.Test;
3737
import org.junit.runner.RunWith;
3838
import org.junit.runners.JUnit4;
@@ -50,17 +50,19 @@
5050
* operations are linearizable in each interleave scenario.
5151
*/
5252
@ModelCheckingCTest
53-
@OpGroupConfig(name = "update", nonParallel = true)
54-
@OpGroupConfig(name = "write", nonParallel = true)
5553
@Param(name = "keepReady", gen = BooleanGen.class)
5654
@RunWith(JUnit4.class)
57-
public class AsyncServletOutputStreamWriterConcurrencyTest extends VerifierState {
55+
public class AsyncServletOutputStreamWriterConcurrencyTest {
5856
private static final int OPERATIONS_PER_THREAD = 6;
5957

6058
private final AsyncServletOutputStreamWriter writer;
6159
private final boolean[] keepReadyArray = new boolean[OPERATIONS_PER_THREAD];
6260

6361
private volatile boolean isReady;
62+
/**
63+
* The container initiates the first call shortly after {@code startAsync}.
64+
*/
65+
private final AtomicBoolean initialOnWritePossible = new AtomicBoolean(true);
6466
// when isReadyReturnedFalse, writer.onWritePossible() will be called.
6567
private volatile boolean isReadyReturnedFalse;
6668
private int producerIndex;
@@ -71,17 +73,15 @@ public class AsyncServletOutputStreamWriterConcurrencyTest extends VerifierState
7173
public AsyncServletOutputStreamWriterConcurrencyTest() {
7274
BiFunction<byte[], Integer, ActionItem> writeAction =
7375
(bytes, numBytes) -> () -> {
74-
assertWithMessage("write should only be called while isReady() is true")
75-
.that(isReady)
76-
.isTrue();
76+
assertTrue("write should only be called while isReady() is true", isReady);
7777
// The byte to be written must equal to consumerIndex, otherwise execution order is wrong
78-
assertWithMessage("write in wrong order").that(bytes[0]).isEqualTo((byte) consumerIndex);
78+
assertEquals("write in wrong order", bytes[0], (byte) consumerIndex);
7979
bytesWritten++;
8080
writeOrFlush();
8181
};
8282

8383
ActionItem flushAction = () -> {
84-
assertWithMessage("flush must only be called while isReady() is true").that(isReady).isTrue();
84+
assertTrue("flush must only be called while isReady() is true", isReady);
8585
writeOrFlush();
8686
};
8787

@@ -102,12 +102,13 @@ private void writeOrFlush() {
102102
}
103103

104104
private boolean isReady() {
105-
if (!isReady) {
106-
assertWithMessage("isReady() already returned false, onWritePossible() will be invoked")
107-
.that(isReadyReturnedFalse).isFalse();
105+
boolean copyOfIsReady = isReady;
106+
if (!copyOfIsReady) {
107+
assertFalse("isReady() already returned false, onWritePossible() will be invoked",
108+
isReadyReturnedFalse);
108109
isReadyReturnedFalse = true;
109110
}
110-
return isReady;
111+
return copyOfIsReady;
111112
}
112113

113114
/**
@@ -118,7 +119,7 @@ private boolean isReady() {
118119
* the ServletOutputStream should become unready if keepReady == false.
119120
*/
120121
// @com.google.errorprone.annotations.Keep
121-
@Operation(group = "write")
122+
@Operation(nonParallelGroup = "write")
122123
public void write(@Param(name = "keepReady") boolean keepReady) throws IOException {
123124
keepReadyArray[producerIndex] = keepReady;
124125
writer.writeBytes(new byte[]{(byte) producerIndex}, 1);
@@ -133,7 +134,7 @@ public void write(@Param(name = "keepReady") boolean keepReady) throws IOExcepti
133134
* the ServletOutputStream should become unready if keepReady == false.
134135
*/
135136
// @com.google.errorprone.annotations.Keep // called by lincheck reflectively
136-
@Operation(group = "write")
137+
@Operation(nonParallelGroup = "write")
137138
public void flush(@Param(name = "keepReady") boolean keepReady) throws IOException {
138139
keepReadyArray[producerIndex] = keepReady;
139140
writer.flush();
@@ -142,17 +143,26 @@ public void flush(@Param(name = "keepReady") boolean keepReady) throws IOExcepti
142143

143144
/** If the writer is not ready, let it turn ready and call writer.onWritePossible(). */
144145
// @com.google.errorprone.annotations.Keep // called by lincheck reflectively
145-
@Operation(group = "update")
146+
@Operation(nonParallelGroup = "update")
146147
public void maybeOnWritePossible() throws IOException {
147-
if (isReadyReturnedFalse) {
148+
if (initialOnWritePossible.compareAndSet(true, false)) {
149+
isReady = true;
150+
writer.onWritePossible();
151+
} else if (isReadyReturnedFalse) {
148152
isReadyReturnedFalse = false;
149153
isReady = true;
150154
writer.onWritePossible();
151155
}
152156
}
153157

154158
@Override
155-
protected Object extractState() {
159+
public final boolean equals(Object o) {
160+
return o instanceof AsyncServletOutputStreamWriterConcurrencyTest
161+
&& bytesWritten == ((AsyncServletOutputStreamWriterConcurrencyTest) o).bytesWritten;
162+
}
163+
164+
@Override
165+
public int hashCode() {
156166
return bytesWritten;
157167
}
158168

@@ -169,6 +179,6 @@ public void linCheck() {
169179
AtomicReference.class.getName())
170180
.allMethods()
171181
.treatAsAtomic());
172-
LinChecker.check(AsyncServletOutputStreamWriterConcurrencyTest.class, options);
182+
options.check(AsyncServletOutputStreamWriterConcurrencyTest.class);
173183
}
174184
}

0 commit comments

Comments
 (0)