grpc / grpc/grpc-java

NPE in RetriableStream when ClientCallStreamObserver.cancel() races ClientCall.start()

Đang mở
#12,964 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Java
Star
12.1k
Fork
4k
Merge trung bình
2 ngày 17 giờ
Pull request đã merge (30 ngày)
37

Mô tả

### What version of gRPC-Java are you using?

1.64.0. The relevant code is unchanged in 1.69.1 and 1.83.1.

### What is your environment?

Reproduced on JDK 21 (macOS). Originally found on Android (grpc-android / grpc-okhttp), where it
causes a fatal crash at meaningful volume in production.

### What did you do?

Cancel a unary call through the `ClientCallStreamObserver` handed to
`ClientResponseObserver.beforeStart()`, from a different thread, while `ClientCall.start()` is
executing.

This is what an application does when it ties call cancellation to some external lifecycle — in our
case a Kotlin coroutine cancellation handler cancels the in-flight call when the caller goes away.

### What did you expect to see?

The call cancelled, or the cancel ignored. `ClientCallImpl.cancelInternal` already guards against a
not-yet-started call (`if (stream != null)`), so cancelling before `start()` is safe and throws
nothing.

### What did you see instead?

An NPE thrown back out of `cancel()` on the calling thread, wrapped as a `StatusRuntimeException`:

```
io.grpc.StatusRuntimeException: UNKNOWN: Uncaught exception in the SynchronizationContext. Re-thrown.
at io.grpc.Status.asRuntimeException(Status.java:525)
at io.grpc.internal.RetriableStream$1.uncaughtException(RetriableStream.java:75)
at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:96)
at io.grpc.SynchronizationContext.execute(SynchronizationContext.java:126)
at io.grpc.internal.RetriableStream.safeCloseMasterListener(RetriableStream.java:838)
at io.grpc.internal.RetriableStream.cancel(RetriableStream.java:531)
at io.grpc.internal.ClientCallImpl.cancelInternal(ClientCallImpl.java:480)
at io.grpc.internal.ClientCallImpl.cancel(ClientCallImpl.java:454)
at io.grpc.stub.ClientCalls$CallToStreamObserverAdapter.cancel(ClientCalls.java:431)
...
Caused by: java.lang.NullPointerException: Cannot invoke
"io.grpc.internal.ClientStreamListener.closed(io.grpc.Status, io.grpc.internal.ClientStreamListener$RpcProgress, io.grpc.Metadata)"
because the return value of "io.grpc.internal.RetriableStream.access$700(io.grpc.internal.RetriableStream)" is null
at io.grpc.internal.RetriableStream$4.run(RetriableStream.java:843)
at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94)
```

### Analysis

`ClientCallImpl.startInternal` assigns the stream at line 250 and starts it at line 285:

```java
stream = clientStreamProvider.newStream(method, callOptions, headers, context); // :250
...
stream.start(new ClientStreamListenerImpl(observer)); // :285
```

`RetriableStream.masterListener` is assigned as the first statement of `RetriableStream.start()`
(`RetriableStream.java:388`). So between those two lines the stream is non-null but `masterListener`
is still null.

A `cancel()` arriving in that window passes the `stream != null` guard in `cancelInternal`, reaches
`RetriableStream.cancel` :531 and `safeCloseMasterListener` :838, whose Runnable dereferences the
null `masterListener` at :843. Because `listenerSerializeExecutor` is a `SynchronizationContext`
(`RetriableStream.java:69`) and `SynchronizationContext.execute` is `executeLater(task); drain()`,
the Runnable runs inline on the cancelling thread, and the uncaught handler at
`RetriableStream.java:70-76` rethrows. The exception therefore surfaces on the caller's thread
rather than being contained.

The window is only reachable from application code because `ClientCalls` hands out the
`ClientCallStreamObserver` through `ClientResponseObserver.beforeStart()`, which runs in the
`StreamObserverToCallListenerAdapter` constructor (`ClientCalls.java:451`) — before `startCall()`
calls `call.start()` (`ClientCalls.java:311`). gRPC defers its own cancellation sources past
`start()` for exactly this reason, per the comment at `ClientCallImpl.java:287`:

> Delay any sources of cancellation after start(), because most of the transports are broken if they
> receive cancel before start. Issue #1343 has more details

but an application cancel arriving through the stub API gets none of that protection.

Only affects channels with retries enabled (the default), since `RetriableStream` is otherwise not
in the path.

### Reproducer

Races the window and hits it within a few hundred attempts. No server is needed — the crash happens
before the transport is involved.

```kotlin
private object StringMarshaller : MethodDescriptor.Marshaller {
override fun stream(value: String): InputStream = ByteArrayInputStream(value.toByteArray())
override fun parse(stream: InputStream): String = stream.readBytes().decodeToString()
}

private val METHOD: MethodDescriptor = MethodDescriptor.newBuilder()
.setType(MethodDescriptor.MethodType.UNARY)
.setFullMethodName("repro.CancelRace/Start")
.setRequestMarshaller(StringMarshaller)
.setResponseMarshaller(StringMarshaller)
.build()

val channel = OkHttpChannelBuilder.forAddress("localhost", 1).usePlaintext().build()
val canceller = Executors.newSingleThreadExecutor()

repeat(20_000) { attempt ->
val stream = AtomicReference?>(null)
val release = AtomicBoolean(false)

val task = canceller.submit {
while (!release.get()) Thread.onSpinWait()
repeat(attempt % 64) { Thread.onSpinWait() } // sweep the offset across attempts
stream.get()?.cancel("cancel racing start", null) // <-- throws here
}

val observer = object : ClientResponseObserver {
override fun beforeStart(requestStream: ClientCallStreamObserver) {
stream.set(requestStream)
release.set(true)
}
override fun onNext(value: String) = Unit
override fun onError(t: Throwable) = Unit
override fun onCompleted() = Unit
}

try {
ClientCalls.asyncUnaryCall(channel.newCall(METHOD, CallOptions.DEFAULT), "request", observer)
} catch (_: IllegalStateException) {
// cancel landed before start(): "call was cancelled". Not the bug.
}
task.get()
}
```

For a fully deterministic demonstration, a breakpoint at `RetriableStream.java:388` with a
thread-only suspend policy holds the window open indefinitely; cancelling while it is parked
reproduces it every time.

### Suggested direction

Either null-check `masterListener` in `safeCloseMasterListener` and fall back to
`savedCloseMasterListenerReason` once `start()` attaches the listener, or have
`CallToStreamObserverAdapter.cancel()` defer until the call has started, mirroring how
`ClientCallImpl` already defers its own cancellation sources past `stream.start()`.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu bằng cách đọc ClientCallImpl.startInternal và RetriableStream.start, cancel, cùng safeCloseMasterListener, sau đó chạy Kotlin reproducer được cung cấp với retries được bật. So sánh hai hướng được đề xuất và xác minh rằng việc hủy cạnh tranh với ClientCall.start không còn ném ra NPE, trong khi lệnh gọi được hủy hoặc được bỏ qua một cách an toàn.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
java, kotlin
Lĩnh vực
api, backend-api-design
Loại issue
Lỗi
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
48/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.