Skip to content

Improve refcounting in testClientCancellation #110309

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

Merged
DaveCTurner merged 2 commits into from
Jul 1, 2024
Merged

Improve refcounting in testClientCancellation #110309

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56a5ae0
Improve refcounting in `testClientCancellation`
DaveCTurner Jun 30, 2024
9cd65bb
Merge branch 'main' into 2024/06/30/improve-Netty4ChunkedContinuation…
DaveCTurner Jun 30, 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
24 changes: 8 additions & 16 deletions .../internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,8 @@
import org.elasticsearch.tasks.Task;
import org.elasticsearch.tasks.TaskCancelledException;
import org.elasticsearch.test.MockLog;
import org.elasticsearch.test.ReachabilityChecker;
import org.elasticsearch.test.junit.annotations.TestLogging;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.LeakTracker;
import org.elasticsearch.transport.TransportService;
import org.elasticsearch.transport.netty4.Netty4Utils;
import org.elasticsearch.xcontent.ToXContentObject;
Expand Down Expand Up @@ -317,20 +315,14 @@ public void onFailure(Exception exception) {

private static Releasable withResourceTracker() {
assertNull(refs);
final ReachabilityChecker reachabilityChecker = new ReachabilityChecker();
final var latch = new CountDownLatch(1);
refs = LeakTracker.wrap(reachabilityChecker.register(AbstractRefCounted.of(latch::countDown)));
refs = AbstractRefCounted.of(latch::countDown);
Copy link
Copy Markdown
Contributor

@nicktindall nicktindall Jun 30, 2024

Choose a reason for hiding this comment

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

Are you sure it's not worth leaving the leak tracking in? it did make it much easier to troubleshoot.

Copy link
Copy Markdown
Member Author

@DaveCTurner DaveCTurner Jul 1, 2024

Choose a reason for hiding this comment

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

I'm not sure, no, although I'd generally rather keep debug stuff out of tests while they're not known to be flaky. If we keep having problems here then we can put it back easily enough.

return () -> {
refs.decRef();
boolean success = false;
try {
safeAwait(latch);
success = true;
} finally {
refs = null;
if (success == false) {
reachabilityChecker.ensureUnreachable();
}
}
};
}
Expand Down Expand Up @@ -643,14 +635,11 @@ public void close() {

@Override
public void accept(RestChannel channel) {
client.execute(TYPE, new Request(), new RestActionListener<>(channel) {
localRefs.mustIncRef();
client.execute(TYPE, new Request(), ActionListener.releaseAfter(new RestActionListener<>(channel) {
@Override
protected void processResponse(Response response) {
// incRef can fail if the request was already cancelled
if (localRefs.tryIncRef() == false) {
assert localRefs.hasReferences() == false : "tryIncRef failed but RefCounted not completed";
return;
}
localRefs.mustIncRef();
channel.sendResponse(RestResponse.chunked(RestStatus.OK, response.getResponseBodyPart(), () -> {
// cancellation notification only happens while processing a continuation, not while computing
// the next one; prompt cancellation requires use of something like RestCancellableNodeClient
Expand All @@ -659,7 +648,10 @@ protected void processResponse(Response response) {
localRefs.decRef();
}));
}
});
}, () -> {
assertSame(localRefs, refs);
localRefs.decRef();
}));
}
};
} else {
Expand Down