Skip to content

Commit 78107a6

Browse files
ejona86ejona86
committed
core: Do not rely on HTTP 200
We only want to use the HTTP code for errors, when the response is not grpc. grpc status codes may be mapped to HTTP codes in the future, and we don't want to break when that happens. We also don't want to ever accidentally use Status.OK without receiving it from the server, even for HTTP 200.
1 parent c38611a commit 78107a6

File tree

6 files changed

+463
-112
lines changed

6 files changed

+463
-112
lines changed

‎core/src/main/java/io/grpc/internal/GrpcUtil.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public static Status httpStatusToGrpcStatus(int httpStatusCode) {
185185
}
186186
if (httpStatusCode < 300) {
187187
// 2xx
188-
return Status.OK;
188+
return Status.UNKNOWN;
189189
}
190190
return Status.UNKNOWN;
191191
}

‎core/src/main/java/io/grpc/internal/Http2ClientStream.java‎

Lines changed: 57 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public Integer parseAsciiString(byte[] serialized) {
7979
private Status transportError;
8080
private Metadata transportErrorMetadata;
8181
private Charset errorCharset = Charsets.UTF_8;
82-
private boolean contentTypeChecked;
82+
private boolean headersReceived;
8383

8484
protected Http2ClientStream(WritableBufferAllocator bufferAllocator, int maxMessageSize,
8585
StatsTraceContext statsTraceCtx) {
@@ -94,28 +94,37 @@ protected Http2ClientStream(WritableBufferAllocator bufferAllocator, int maxMess
9494
protected void transportHeadersReceived(Metadata headers) {
9595
Preconditions.checkNotNull(headers, "headers");
9696
if (transportError != null) {
97-
// Already received a transport error so just augment it.
98-
transportError = transportError.augmentDescription(headers.toString());
97+
// Already received a transport error so just augment it. Something is really, really strange.
98+
transportError = transportError.augmentDescription("headers: " + headers);
9999
return;
100100
}
101-
Status httpStatus = statusFromHttpStatus(headers);
102-
if (httpStatus == null) {
103-
transportError = Status.INTERNAL.withDescription(
104-
"received non-terminal headers with no :status");
105-
} else if (!httpStatus.isOk()) {
106-
transportError = httpStatus;
107-
} else {
108-
transportError = checkContentType(headers);
109-
}
110-
if (transportError != null) {
111-
// Note we don't immediately report the transport error, instead we wait for more data on the
112-
// stream so we can accumulate more detail into the error before reporting it.
113-
transportError = transportError.augmentDescription("\n" + headers);
114-
transportErrorMetadata = headers;
115-
errorCharset = extractCharset(headers);
116-
} else {
101+
try {
102+
if (headersReceived) {
103+
transportError = Status.INTERNAL.withDescription("Received headers twice");
104+
return;
105+
}
106+
Integer httpStatus = headers.get(HTTP2_STATUS);
107+
if (httpStatus != null && httpStatus >= 100 && httpStatus < 200) {
108+
// Ignore the headers. See RFC 7540 §8.1
109+
return;
110+
}
111+
headersReceived = true;
112+
113+
transportError = validateInitialMetadata(headers);
114+
if (transportError != null) {
115+
return;
116+
}
117+
117118
stripTransportDetails(headers);
118119
inboundHeadersReceived(headers);
120+
} finally {
121+
if (transportError != null) {
122+
// Note we don't immediately report the transport error, instead we wait for more data on
123+
// the stream so we can accumulate more detail into the error before reporting it.
124+
transportError = transportError.augmentDescription("headers: " + headers);
125+
transportErrorMetadata = headers;
126+
errorCharset = extractCharset(headers);
127+
}
119128
}
120129
}
121130

@@ -162,14 +171,14 @@ protected void transportDataReceived(ReadableBuffer frame, boolean endOfStream)
162171
*/
163172
protected void transportTrailersReceived(Metadata trailers) {
164173
Preconditions.checkNotNull(trailers, "trailers");
165-
if (transportError != null) {
166-
// Already received a transport error so just augment it.
167-
transportError = transportError.augmentDescription(trailers.toString());
168-
} else {
169-
transportError = checkContentType(trailers);
170-
transportErrorMetadata = trailers;
174+
if (transportError == null && !headersReceived) {
175+
transportError = validateInitialMetadata(trailers);
176+
if (transportError != null) {
177+
transportErrorMetadata = trailers;
178+
}
171179
}
172180
if (transportError != null) {
181+
transportError = transportError.augmentDescription("trailers: " + trailers);
173182
inboundTransportError(transportError, transportErrorMetadata);
174183
sendCancel(Status.CANCELLED);
175184
} else {
@@ -179,50 +188,44 @@ protected void transportTrailersReceived(Metadata trailers) {
179188
}
180189
}
181190

182-
private static Status statusFromHttpStatus(Metadata metadata) {
183-
Integer httpStatus = metadata.get(HTTP2_STATUS);
184-
if (httpStatus != null) {
185-
Status status = GrpcUtil.httpStatusToGrpcStatus(httpStatus);
186-
return status.isOk() ? status
187-
: status.augmentDescription("extracted status from HTTP :status " + httpStatus);
188-
}
189-
return null;
190-
}
191-
192191
/**
193192
* Extract the response status from trailers.
194193
*/
195194
private Status statusFromTrailers(Metadata trailers) {
196195
Status status = trailers.get(Status.CODE_KEY);
197-
if (status == null) {
198-
status = statusFromHttpStatus(trailers);
199-
if (status == null || status.isOk()) {
200-
status = Status.UNKNOWN.withDescription("missing GRPC status in response");
201-
} else {
202-
status = status.withDescription(
203-
"missing GRPC status, inferred error from HTTP status code");
204-
}
196+
if (status != null) {
197+
return status.withDescription(trailers.get(Status.MESSAGE_KEY));
198+
}
199+
// No status; something is broken. Try to provide a resonanable error.
200+
if (headersReceived) {
201+
return Status.UNKNOWN.withDescription("missing GRPC status in response");
205202
}
206-
String message = trailers.get(Status.MESSAGE_KEY);
207-
if (message != null) {
208-
status = status.augmentDescription(message);
203+
Integer httpStatus = trailers.get(HTTP2_STATUS);
204+
if (httpStatus != null) {
205+
status = GrpcUtil.httpStatusToGrpcStatus(httpStatus);
206+
} else {
207+
status = Status.INTERNAL.withDescription("missing HTTP status code");
209208
}
210-
return status;
209+
return status.augmentDescription(
210+
"missing GRPC status, inferred error from HTTP status code");
211211
}
212212

213213
/**
214-
* Inspect the content type field from received headers or trailers and return an error Status if
215-
* content type is invalid or not present. Returns null if no error was found.
214+
* Inspect initial headers to make sure they conform to HTTP and gRPC, returning a {@code Status}
215+
* on failure.
216+
*
217+
* @return status with description of failure, or {@code null} when valid
216218
*/
217219
@Nullable
218-
private Status checkContentType(Metadata headers) {
219-
if (contentTypeChecked) {
220-
return null;
220+
private Status validateInitialMetadata(Metadata headers) {
221+
Integer httpStatus = headers.get(HTTP2_STATUS);
222+
if (httpStatus == null) {
223+
return Status.INTERNAL.withDescription("Missing HTTP status code");
221224
}
222-
contentTypeChecked = true;
223225
String contentType = headers.get(GrpcUtil.CONTENT_TYPE_KEY);
224226
if (!GrpcUtil.isGrpcContentType(contentType)) {
225-
return Status.INTERNAL.withDescription("Invalid content-type: " + contentType);
227+
return GrpcUtil.httpStatusToGrpcStatus(httpStatus)
228+
.augmentDescription("invalid content-type: " + contentType);
226229
}
227230
return null;
228231
}

‎core/src/main/java/io/grpc/internal/Http2ClientStreamTransportState.java‎

Lines changed: 58 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public Integer parseAsciiString(byte[] serialized) {
7979
private Status transportError;
8080
private Metadata transportErrorMetadata;
8181
private Charset errorCharset = Charsets.UTF_8;
82-
private boolean contentTypeChecked;
82+
private boolean headersReceived;
8383

8484
protected Http2ClientStreamTransportState(int maxMessageSize, StatsTraceContext statsTraceCtx) {
8585
super(maxMessageSize, statsTraceCtx);
@@ -99,28 +99,37 @@ protected Http2ClientStreamTransportState(int maxMessageSize, StatsTraceContext
9999
protected void transportHeadersReceived(Metadata headers) {
100100
Preconditions.checkNotNull(headers, "headers");
101101
if (transportError != null) {
102-
// Already received a transport error so just augment it.
103-
transportError = transportError.augmentDescription(headers.toString());
102+
// Already received a transport error so just augment it. Something is really, really strange.
103+
transportError = transportError.augmentDescription("headers: " + headers);
104104
return;
105105
}
106-
Status httpStatus = statusFromHttpStatus(headers);
107-
if (httpStatus == null) {
108-
transportError = Status.INTERNAL.withDescription(
109-
"received non-terminal headers with no :status");
110-
} else if (!httpStatus.isOk()) {
111-
transportError = httpStatus;
112-
} else {
113-
transportError = checkContentType(headers);
114-
}
115-
if (transportError != null) {
116-
// Note we don't immediately report the transport error, instead we wait for more data on the
117-
// stream so we can accumulate more detail into the error before reporting it.
118-
transportError = transportError.augmentDescription("\n" + headers);
119-
transportErrorMetadata = headers;
120-
errorCharset = extractCharset(headers);
121-
} else {
106+
try {
107+
if (headersReceived) {
108+
transportError = Status.INTERNAL.withDescription("Received headers twice");
109+
return;
110+
}
111+
Integer httpStatus = headers.get(HTTP2_STATUS);
112+
if (httpStatus != null && httpStatus >= 100 && httpStatus < 200) {
113+
// Ignore the headers. See RFC 7540 §8.1
114+
return;
115+
}
116+
headersReceived = true;
117+
118+
transportError = validateInitialMetadata(headers);
119+
if (transportError != null) {
120+
return;
121+
}
122+
122123
stripTransportDetails(headers);
123124
inboundHeadersReceived(headers);
125+
} finally {
126+
if (transportError != null) {
127+
// Note we don't immediately report the transport error, instead we wait for more data on
128+
// the stream so we can accumulate more detail into the error before reporting it.
129+
transportError = transportError.augmentDescription("headers: " + headers);
130+
transportErrorMetadata = headers;
131+
errorCharset = extractCharset(headers);
132+
}
124133
}
125134
}
126135

@@ -159,14 +168,14 @@ protected void transportDataReceived(ReadableBuffer frame, boolean endOfStream)
159168
*/
160169
protected void transportTrailersReceived(Metadata trailers) {
161170
Preconditions.checkNotNull(trailers, "trailers");
162-
if (transportError != null) {
163-
// Already received a transport error so just augment it.
164-
transportError = transportError.augmentDescription(trailers.toString());
165-
} else {
166-
transportError = checkContentType(trailers);
167-
transportErrorMetadata = trailers;
171+
if (transportError == null && !headersReceived) {
172+
transportError = validateInitialMetadata(trailers);
173+
if (transportError != null) {
174+
transportErrorMetadata = trailers;
175+
}
168176
}
169177
if (transportError != null) {
178+
transportError = transportError.augmentDescription("trailers: " + trailers);
170179
http2ProcessingFailed(transportError, transportErrorMetadata);
171180
} else {
172181
Status status = statusFromTrailers(trailers);
@@ -175,50 +184,44 @@ protected void transportTrailersReceived(Metadata trailers) {
175184
}
176185
}
177186

178-
private static Status statusFromHttpStatus(Metadata metadata) {
179-
Integer httpStatus = metadata.get(HTTP2_STATUS);
180-
if (httpStatus != null) {
181-
Status status = GrpcUtil.httpStatusToGrpcStatus(httpStatus);
182-
return status.isOk() ? status
183-
: status.augmentDescription("extracted status from HTTP :status " + httpStatus);
184-
}
185-
return null;
186-
}
187-
188187
/**
189188
* Extract the response status from trailers.
190189
*/
191-
private static Status statusFromTrailers(Metadata trailers) {
190+
private Status statusFromTrailers(Metadata trailers) {
192191
Status status = trailers.get(Status.CODE_KEY);
193-
if (status == null) {
194-
status = statusFromHttpStatus(trailers);
195-
if (status == null || status.isOk()) {
196-
status = Status.UNKNOWN.withDescription("missing GRPC status in response");
197-
} else {
198-
status = status.withDescription(
199-
"missing GRPC status, inferred error from HTTP status code");
200-
}
192+
if (status != null) {
193+
return status.withDescription(trailers.get(Status.MESSAGE_KEY));
194+
}
195+
// No status; something is broken. Try to provide a resonanable error.
196+
if (headersReceived) {
197+
return Status.UNKNOWN.withDescription("missing GRPC status in response");
201198
}
202-
String message = trailers.get(Status.MESSAGE_KEY);
203-
if (message != null) {
204-
status = status.augmentDescription(message);
199+
Integer httpStatus = trailers.get(HTTP2_STATUS);
200+
if (httpStatus != null) {
201+
status = GrpcUtil.httpStatusToGrpcStatus(httpStatus);
202+
} else {
203+
status = Status.INTERNAL.withDescription("missing HTTP status code");
205204
}
206-
return status;
205+
return status.augmentDescription(
206+
"missing GRPC status, inferred error from HTTP status code");
207207
}
208208

209209
/**
210-
* Inspect the content type field from received headers or trailers and return an error Status if
211-
* content type is invalid or not present. Returns null if no error was found.
210+
* Inspect initial headers to make sure they conform to HTTP and gRPC, returning a {@code Status}
211+
* on failure.
212+
*
213+
* @return status with description of failure, or {@code null} when valid
212214
*/
213215
@Nullable
214-
private Status checkContentType(Metadata headers) {
215-
if (contentTypeChecked) {
216-
return null;
216+
private Status validateInitialMetadata(Metadata headers) {
217+
Integer httpStatus = headers.get(HTTP2_STATUS);
218+
if (httpStatus == null) {
219+
return Status.INTERNAL.withDescription("Missing HTTP status code");
217220
}
218-
contentTypeChecked = true;
219221
String contentType = headers.get(GrpcUtil.CONTENT_TYPE_KEY);
220222
if (!GrpcUtil.isGrpcContentType(contentType)) {
221-
return Status.INTERNAL.withDescription("Invalid content-type: " + contentType);
223+
return GrpcUtil.httpStatusToGrpcStatus(httpStatus)
224+
.augmentDescription("invalid content-type: " + contentType);
222225
}
223226
return null;
224227
}

0 commit comments

Comments
 (0)