Give ResponseStore for codeactions a minimum history size of 8

[ethan-vanderheijden]

Fixes #3180

I'm not entirely clear on why history size is tied to processor count. Is it because a client with more processors tends to issue more textDocument/codeAction requests? Either way, setting a minimum history size shouldn't cause any problems. Selecting an item from the source action menu tends to issue 2-4 requests, so I chose 8 as a safe minimum size.

[eclipse-ls-bot]

Can one of the admins verify this patch?

[robstryker]

Looks reasonable to me unless there's some other reason to reject, @rgrunber ?

[rgrunber]

No, I just had a question in the issue itself for Jinbo regarding why it was done this way. My best guess is they assumed that LSP4J would only allow as many parallel requests on a thread pool as parallelism value, but clearly that's not the case.

[testforstephen]

No, I just had a question in the issue itself for Jinbo regarding why it was done this way. My best guess is they assumed that LSP4J would only allow as many parallel requests on a thread pool as parallelism value, but clearly that's not the case.

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/JDTLanguageServer.java

Lines 745 to 756 in 161815b

	public CompletableFuture<List<Either<Command, CodeAction>>> codeAction(CodeActionParams params) {
	debugTrace(">> document/codeAction");
	CodeActionHandler handler = new CodeActionHandler(this.preferenceManager);
	return computeAsync((monitor) -> {
	waitForLifecycleJobs(monitor);
	// see https://github.com/eclipse-jdtls/eclipse.jdt.ls/issues/2799
	// Optimize the performance of the code actions
	synchronized (codeActionLock) {
	return handler.getCodeActionCommands(params, monitor);
	}
	});
	}

See the specific implementation for computeAsync, which runs at the CompletableFuture.defaultExecutor() (aka. ForkJoinPool.commonPool()). That's why we set the response store size to be same as the commonPool.

But looks into the implementation for CompletableFuture.defaultExecutor() again, when ForkJoinPool.getCommonPoolParallelism() equals 1, defaultExecutor will fall back to ThreadPerTaskExecutor(). That should be the reason why it fails at #3180.

    // snippets from java.util.concurrent.CompletableFuture

    private static final boolean USE_COMMON_POOL =
        (ForkJoinPool.getCommonPoolParallelism() > 1);

    /**
     * Default executor -- ForkJoinPool.commonPool() unless it cannot
     * support parallelism.
     */
    private static final Executor ASYNC_POOL = USE_COMMON_POOL ?
        ForkJoinPool.commonPool() : new ThreadPerTaskExecutor();

    public Executor defaultExecutor() {
        return ASYNC_POOL;
    }

[testforstephen]

Using a magic number is acceptable when parallelism fails. However, it’s better to add a comment in the code explaining the reason behind the magic number. This will provide context for future developers in case the magic number causes issues again.

[rgrunber]

@ethan-vanderheijden let's adjust the value to (ForkJoinPool.getCommonPoolParallelism() > 1) : ForkJoinPool.commonPool().getParallelism() : 8 ? Also we can probably use ForkJoinPool.getCommonPoolParallelism() assuming it's the same. I would also add a comment about this on the declaration regarding the value, as Jinbo mentions.

[ethan-vanderheijden]

@rgrunber
Something doesn't add up. If I modify computeAsync to force every request to run synchronously, I can still reproduce the issue. I don't think asynchronous handling of textDocument/codeAction requests is the root cause.

I think your fundamental assumption is that only the most recent textDocument/codeAction request will be resolved further, but this is not always true. Here's my LSP stack trace:

--- Sending request ---
{
    id: 5,
    method: "textDocument/codeAction",
}

--- Receiving response ---
{
    id: 5,
    result: [
        {
            title: "Organize imports"
            data: {
                pid: 0,
                rid: 13
            },
            ...
        },
        ...
    ],
}

--- Sending request ---
{
    id: 6,
    method: "textDocument/codeAction",
}

--- Receiving response ---
{
    id: 6,
    result: [
        {
            title: "Add Javadoc comment"
            data: {
                pid: 0,
                rid: 14
            },
            ...
        },
        ...
    ],
}

--- Sending request ---
{
    id: 7,
    method: "codeAction/resolve",
    params: {
        data: {
            pid: 0,
            rid: 13,
        },
        ...
    }
}

Notice that it is trying to resolve rid: 13, which is not the most recent textDocument/codeAction request.

I should clarify how to reproduce this situation:

1. Load a file with the following content

public class Main {
    public static void main(String[] args) throws InterruptedException {
        LinkedList<Integer> list = new LinkedList<>();
    }
}

2. Place your cursor on line 1
3. Right-click line 3 and immediately select "Source Actions"
4. Select something from the Source Actions menu

It will send the first textDocument/codeAction when you open the Source Actions menu. Since your cursor has now moved to line 3, it will send a second textDocument/codeAction to get code actions for your cursor position. When you select a Source Action, it will resolve the first code action request even though it is no longer the most recent code action request.

The issue is that opening Source Actions and moving your cursor will both produce a textDocument/codeAction, and either one may be resolved in the future. The JDTLS should not assume that only the latest code action request will be resolved.

[rgrunber]

Thanks. I was able to reproduce it. I did have to manually set my code action storage size to 1 and I triggered the "Source Action" menu using the keybinding immediately after left-clicking, since I couldn't figure out a way to right click fast enough.

Jun. 19, 2024 5:08:13 P.M. org.eclipse.lsp4j.jsonrpc.RemoteEndpoint fallbackResponseError
SEVERE: Internal error: java.lang.IllegalStateException: Invalid codeAction proposal
java.util.concurrent.CompletionException: java.lang.IllegalStateException: Invalid codeAction proposal
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.lang.IllegalStateException: Invalid codeAction proposal
	at org.eclipse.jdt.ls.core.internal.handlers.CodeActionResolveHandler.resolve(CodeActionResolveHandler.java:44)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$15(JDTLanguageServer.java:781)
	at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:87)
	... 7 more

So you're saying we aren't dependent on the number of concurrent threads that the thread pool can support, but rather the number of concurrent requests that might arise from the UI.

[ethan-vanderheijden]

The number of concurrent requests from the UI affects history size, but so does the number of threads. For instance, if we wanted to store the most recent code action response, but 4 requests are processed in parallel, and we don’t know which will be done processing first, then worst case scenario, we need to store the last 4 results. If you want to calculate history size, the following formula should work:

# of threads + # of concurrent UI requests - 1

However, if “# of threads” is 1, then it switches to ThreadPerTaskExecutor which means unlimited code action requests can be processed in parallel. Here, we can use a magic number (e.g. assume up to 8 requests will process in parallel in a normal workload).

All of this is over-engineered. I suggest hardcoding history size to a large number like 32. Are ChangeCorrectionProposalCore/CodeActionProposal very large objects? If so, we might have to be careful how many we retain in memory.

[rgrunber]

Alright, then I'm fine with what you currently have, which is Math.max(ForkJoinPool.commonPool().getParallelism(), 8). I can't see a case where the UI would result in more than 8 parallel code action requests. I would just add a comment explaining what the 2 variables are that affect the total number of parallel code action requests.

I'd also just change the commit message to :

Give ResponseStore for code actions a minimum history size of 8.