fix: don't let a slow write stall unrelated LSP requests - #1638
Conversation
| private final ExecutorService messageWriter; | ||
| private final ExecutorService requestWriter; | ||
| // Tracks the most recently enqueued notification write so requests can wait for it, preserving relative order. | ||
| private volatile CompletableFuture<Void> lastNotificationWrite = CompletableFuture.completedFuture(null); |
There was a problem hiding this comment.
The volatile keyword guarantees visibility but not atomicity of the read-then-chain sequence. Scenario:
- Thread A (request) reads lastNotificationWrite → gets future F1
- Thread B (notification) updates lastNotificationWrite → F2
- Thread A chains .thenRunAsync(...) on F1 (the old one), not F2
In practice this is acceptable (the request was submitted before notification F2), but an
AtomicReference<CompletableFuture> would make the intent more explicit and remove any ambiguity.
| } else { | ||
| write = lastNotificationWrite | ||
| .exceptionally(e -> null) | ||
| .thenRunAsync(() -> consumer.consume(message), requestWriter); |
There was a problem hiding this comment.
requestWriter is a cached thread pool, so multiple requests may call consumer.consume(message) concurrently. This only works because LSP4J's StreamMessageConsumer.consume() uses a synchronized (outputLock) internally.
It would be worth adding a brief comment explaining this dependency, e.g.:
// Safe to call consume() from multiple threads concurrently:
// StreamMessageConsumer.consume() synchronizes on an internal outputLock.
Without this comment, a future contributor might not understand why a multi-threaded pool is safe here.
| // Requests use requestWriter (so a slow one can't stall others) but still wait for prior notifications to be written, to not overtake a didChange they depend on. | ||
| CompletableFuture<Void> write; | ||
| if (message instanceof NotificationMessage) { | ||
| write = CompletableFuture.runAsync(() -> consumer.consume(message), messageWriter); |
There was a problem hiding this comment.
If consumer.consume(message) throws inside the runAsync, the write future completes exceptionally. The next request at line 505 handles this via .exceptionally(e -> null) — good.
However, lastNotificationWrite is updated unconditionally at line 503, even before the write completes. This is
correct behavior (it's a future, not a result), but worth noting: if the messageWriter executor is shut down,
runAsync throws RejectedExecutionException synchronously, and lastNotificationWrite would point to a failed future.
The .exceptionally(e -> null) on line 505 handles this gracefully, so no bug — just worth being aware of.
| .exceptionally(e -> { | ||
| // Requests use requestWriter (so a slow one can't stall others) but still wait for prior notifications to be written, to not overtake a didChange they depend on. | ||
| CompletableFuture<Void> write; | ||
| if (message instanceof NotificationMessage) { |
There was a problem hiding this comment.
There is a subtle gap between line 501 (the runAsync is submitted to messageWriter) and line 503
(lastNotificationWrite = write). During this gap, a concurrent request thread could read the old
lastNotificationWrite and miss this notification entirely.
Consider reversing the order — assign the future first, then submit. One way:
if (message instanceof NotificationMessage) {
CompletableFuture notifWrite = CompletableFuture.runAsync(() -> consumer.consume(message), messageWriter);
lastNotificationWrite = notifWrite;
write = notifWrite;
} else {
Though in practice the gap is negligible since this whole block runs on a single thread (the LSP4J dispatch thread),
it's worth confirming that assumption holds.
| private final ExecutorService messageWriter; | ||
| private final ExecutorService requestWriter; | ||
| // Tracks the most recently enqueued notification write so requests can wait for it, preserving relative order. | ||
| private volatile CompletableFuture<Void> lastNotificationWrite = CompletableFuture.completedFuture(null); |
| // Executor service passed through to the LSP4j layer when we attempt to start the LS. It will be used | ||
| // to create a listener that sits on the input stream and processes inbound messages (responses, or server-initiated | ||
| // requests). | ||
| // Notifications (didOpen/didChange/didClose, ...) must be written in order, hence the single thread. |
There was a problem hiding this comment.
// Executor service passed through to the LSP4j layer when we attempt to start the LS. It will be used
// to create a listener that sits on the input stream and processes inbound messages (responses, or server-initiated
// requests).
// Notifications (didOpen/didChange/didClose, ...) must be written in order, hence the single thread.
▎ The original comment (lines 184-186) doesn't accurately describe messageWriter's role — it describes listener. The
▎ new comment at line 187 is correct but contradicts the old one. Consider replacing lines 184-186 entirely:
▎ // Single-threaded executor for writing outgoing notifications (didOpen, didChange, didClose)
▎ // to the language server. Notifications must be written in order, hence the single thread.
▎ // Requests and responses use the separate requestWriter pool.
|
@jorgsowa please rebase your PR from main branch, since I did some changes for didChange just to check that your PR works correctly with my changes. |
2 participants
All outgoing messages (notifications and requests) were serialized through one single-threaded, unbounded messageWriter executor, so a single slow write could pile up every other feature request (hover, completion, ...) against that language server, manifesting as an IDE freeze.
Notifications (
didOpen/didChange/didClose) still need strict ordering, so they stay on messageWriter. Requests/responses now go through a separate pooled executor (requestWriter) since they're independent of each other. Requests still wait for the most recently dispatched notification's write to finish first, so a request can't reach the server ahead of a didChange it depends on.