stub: use the closedTrailers in StatusException #12259
Conversation
| @@ -128,7 +129,7 @@ private RespT read(boolean waitForever, long endNanoTime) | |||
| throw new IllegalStateException( | |||
| "The message disappeared... are you reading from multiple threads?"); | |||
| } else if (!currentClosedStatus.isOk()) { | |||
| throw currentClosedStatus.asException(); | |||
| throw currentClosedStatus.asException(closedTrailers); | |||
There was a problem hiding this comment.
We are assigning member currentStatus variable to local variable currentClosedStatus everywhere because another thread can change it value and there is no synchronisation in our code. Similar concern will apply to metadata trailers as well. In addition we need to ensure that currentStatus and closedTrailers are ready as an atomic whole.
There was a problem hiding this comment.
Should we use something like Pair to contain both status and metadata and call it volatile?
| Preconditions.checkState(closedStatus == null, "ClientCall already closed"); | ||
| closedStatus = status; | ||
| CloseState newCloseState = new CloseState(status, trailers); | ||
| boolean wasSet = closeState.compareAndSet(null, newCloseState); |
There was a problem hiding this comment.
This is the only place assigning the value, so the code was fine with a plain volatile. It only runs on one thread. The volatile is because it is read by other threads, but it is read-only.
| final Metadata trailers; | ||
|
|
||
| CloseState(Status status, Metadata trailers) { | ||
| this.status = status; |
There was a problem hiding this comment.
Let's have a checkNotNull here, for clarity.
Fixes #12256
cc: @benjaminp