Enhance HTTP Status handling #1472

wszydlak · 2025-01-20T13:15:34Z

Pull Request: Enhance HTTP Status handling in unary calls

Background

When using gRPC-Web with a reverse proxy like NGINX, there are scenarios where the proxy may return HTTP status codes (e.g., 503, 505) without invoking the actual gRPC service. In such cases, the gRPC-Web library throws an RpcError with a StatusCode.UNKNOWN. This limits the ability to differentiate between HTTP errors and makes it challenging to implement robust client-side error handling based on specific HTTP statuses.

Changes introduced in this PR

Addition of httpStatusCode to Errors
- Modified the error creation logic in grpcwebclientreadablestream.js to include the httpStatusCode from the XHR object.
- This ensures that the exact HTTP status code is accessible in unary call errors.
- HTTP Status is added as string to fit Metadata type
Enhanced complete event for streaming
- Updated the behavior of the complete event for streaming calls to mirror the unary call changes.
- The httpStatusCode is now included in metadata when firing status event.

Alternatives

As an alternative resolution i can prepare solution similar to this line, so the final result for UNKNOWN error could be handled like:

-    new RpcError(StatusCode.UNKNOWN, 'Unknown Content-type received.'));
+    new RpcError(StatusCode.UNKNOWN, 'Unknown Content-type received, http status code: ' + self.xhr_.getStatus()));

Example behaviour before and after this change

Before:

Unary call errors always return RpcError with StatusCode.UNKNOWN, irrespective of the HTTP status code.

After:

Errors now include the httpStatusCode field in metadata, enabling better decision-making on the client side.

Example:

{
  code: StatusCode.UNKNOWN,
  message: "An error occurred",
  metadata: {
    httpStatusCode: '505'
  }
}

Open Questions

Acceptability: Is adding the httpStatusCode to the RpcError object an acceptable enhancement for unary and streaming calls?
Metadata Handling: Should we consider modifying metadata to accept numbers for better handling of HTTP-specific errors?
Alternative: Is proposed solution ok, or you want to try with proposed alternative?

Why This Change Is Valuable

Improved Client-Side Error Handling: Enables applications to differentiate between HTTP-specific errors (e.g., 503, 505) and gRPC errors, particularly when intermediary layers like NGINX are used.
Consistency: Aligns the behaviour of unary calls with streaming calls, ensuring both provide detailed error context.
Enhanced Debugging: Provides developers with the HTTP status code directly, reducing time spent diagnosing issues caused by reverse proxies.

linux-foundation-easycla · 2025-01-20T13:15:39Z

The committers listed above are authorized under a signed CLA.

✅ login: wszydlak / name: Wojtek Szydlak (b84dc9c)

wszydlak · 2025-01-21T18:06:48Z

@sampajano any chance to move this PR forward?

sampajano

@wszydlak Thank you so much for proposing this change with detailed rationales (even with alternatives)!! Appreciate!

I apologize for the late review! — I was out for a few weeks and have been catching up on things :)

Overall i think this change makes a lot of sense!! And i certainly like it better than the alternative solution — since it provides more structured data and can be better accessed programmatically 😃

Regarding your questions:

Acceptability: Is adding the httpStatusCode to the RpcError object an acceptable enhancement for unary and streaming calls?

I think this is a fairly reasonable improvement to the API!

Metadata Handling: Should we consider modifying metadata to accept numbers for better handling of HTTP-specific errors?

I'm not quite understanding here. Do you mind elaborate a bit (maybe with examples)?

Alternative: Is proposed solution ok, or you want to try with proposed alternative?

I like this solution better since the data is more programmatically accessible!

One question tho:

Are you intending to make this "unary only" feature? Why not make this accessible across the board even for streaming then?

From what I could tell, the following maybe the only other missing scenario where we can return an RpcError from xhr errors:

grpc-web/javascript/net/grpc/web/grpcwebclientreadablestream.js

Lines 260 to 264 in b5ff5d3

    
           if (xhrStatusCode != -1) { 
        
             errorMessage += ', http status code: ' + xhrStatusCode; 
        
           } 
        
           self.handleError_(new RpcError(grpcStatusCode, errorMessage));

Maybe we could simply add httpStatusCode here as well?

And one more thing, could we add a simple test?

Maybe just add a new test like this one?

grpc-web/javascript/net/grpc/web/grpcwebclientbase_test.js

Line 173 in b5ff5d3

async testRpcError() {

Again, thank you so much for proposing this improvement! I think it makes much sense and I appreciate your contribution!

wszydlak · 2025-02-01T19:16:40Z

Hello, thanks for the response

I appreciate that you see this improvement is reasonable.

Are you intending to make this "unary only" feature? Why not make this accessible across the board even for streaming then?

PR contains also changes for the regular streaming: https://github.com/wszydlak/grpc-web/blob/feature/http-status-on-error/javascript/net/grpc/web/grpcwebclientreadablestream.js#L265

Metadata Handling: Should we consider modifying metadata to accept numbers for better handling of HTTP-specific errors?

I'm not quite understanding here. Do you mind elaborate a bit (maybe with examples)?

Sure, i mean the types in the grpc-web package: https://github.com/wszydlak/grpc-web/blob/feature/http-status-on-error/packages/grpc-web/index.d.ts#L3
Should we keep metadata as Record<string, string> or maybe we could go this way:

export type Metadata = Record<string, string> & { httpStatusCode?: number };

This will allow us to handle httpStatusCode as numbers, not as strings as proposed in current PR.
Current solution converts code to string to be compatible with current Metadata type, but maybe we can make a change here?

And one more thing, could we add a simple test?

Sure, if we will decide what should be the final implementation i will also prepare tests 🙂

sampajano · 2025-02-03T22:40:33Z

Hello, thanks for the response

I appreciate that you see this improvement is reasonable.

Are you intending to make this "unary only" feature? Why not make this accessible across the board even for streaming then?

PR contains also changes for the regular streaming: https://github.com/wszydlak/grpc-web/blob/feature/http-status-on-error/javascript/net/grpc/web/grpcwebclientreadablestream.js#L265

Oh nice! Wonderful! Thanks for reminding me :)

I saw that you've put "unary" in PR title and title and hence the question. Do you mind updating that too?

Metadata Handling: Should we consider modifying metadata to accept numbers for better handling of HTTP-specific errors?

I'm not quite understanding here. Do you mind elaborate a bit (maybe with examples)?

Sure, i mean the types in the grpc-web package: https://github.com/wszydlak/grpc-web/blob/feature/http-status-on-error/packages/grpc-web/index.d.ts#L3 Should we keep metadata as Record<string, string> or maybe we could go this way:
export type Metadata = Record<string, string> & { httpStatusCode?: number };
This will allow us to handle httpStatusCode as numbers, not as strings as proposed in current PR. Current solution converts code to string to be compatible with current Metadata type, but maybe we can make a change here?

Ahh nice.. I like the idea! Why not! :)

And one more thing, could we add a simple test?

Sure, if we will decide what should be the final implementation i will also prepare tests 🙂

Yes. I'm happy with the solution and will merge it once a test is added!

Thanks again! :)

wszydlak · 2025-02-05T13:54:20Z

Hello again @sampajano, coming back with some changes:

When i tried to run mocha tests they didn't work, cause third_party/protobuf/js did not exist, probably using old version, as protobuf/js was moved in v21 to separate protobuf-javascript repository - so i decided to restore mocha test by adding submodule and changing paths.
Changed Metadata to recognize httpStatusCode as number
Modified a little readystatechange event, not to send RpcError when Content-Type of response is not matching application/grpc* - errors will be handled by complete event which for other Content-Type occurs right after readystatechange. This allowed to handle httpStatusCode in single place in code - it will work for unary and streaming
Added tests to verify such cases of receiving Content-Type not matching application/grpc*

I ran the mocha tests and generated test for javascript/net/grpc/web/grpcwebclientbase_test.js

Please confirm that proposed solution is acceptable and if everything looks good or maybe some changes are required.

Additionally, if everything is ok - can we schedule release date of 1.6.0 (assuming this version will contain proposed changes)?

wszydlak · 2025-02-06T06:44:00Z

@sampajano
I see tests failing due to missing new submodule.
I didn't notice this script: https://github.com/grpc/grpc-web/blob/master/scripts/init_submodules.sh which forces to use older protobuf repository version.

I reverted commit with change of the submodule.

Maybe this should be addressed in separate PR.

Waiting for the tests re-run.

wszydlak · 2025-02-11T13:21:35Z

@sampajano I see tests are passing, what are the next steps?

sampajano · 2025-02-16T08:28:26Z

@wszydlak Hi! My apologies that I was caught up in something and not able to find time for this just yet, and will be out until mid next week. I'll review this as soon as I'm back and will get it merged soon!

And yes! We're recently getting some new help on grpc-web now so we'll do a release soon!

wszydlak · 2025-02-20T09:31:56Z

@wszydlak Hi! My apologies that I was caught up in something and not able to find time for this just yet, and will be out until mid next week. I'll review this as soon as I'm back and will get it merged soon!

And yes! We're recently getting some new help on grpc-web now so we'll do a release soon!

Sure, waiting then for a review 👍 Thanks 🙂

sampajano

Hi! Thanks for the change! I've started to review it and have a first few questions! Thanks!

sampajano · 2025-02-22T01:21:41Z

javascript/net/grpc/web/grpcwebclientreadablestream.js

-      let contentType = self.xhr_.getStreamingResponseHeader('Content-Type');
-      if (!contentType) return;
-      contentType = contentType.toLowerCase();
+      const contentType = (self.xhr_.getStreamingResponseHeader('Content-Type') || '').toLowerCase();


Could you explain a bit more on the motivation of all these changes (besides just adding the httpStatusCode in metadata)?

It looks to me that it's altering some existing logic, and I'm not sure if it can cause regressions.

From the PR description, I got the impression that adding httpStatusCode was all we needed.

Is that not the case?

Hi, already described it above: #1472 (comment)

sampajano · 2025-02-22T01:22:40Z

javascript/net/grpc/web/grpcwebclientreadablestream.js

@@ -257,11 +253,14 @@ class GrpcWebClientReadableStream {
          return;
        }
        let errorMessage = ErrorCode.getDebugMessage(lastErrorCode);
+
+        const errorMetadata = /** @type {!StatusMetadata} */ ({});


This line won't pass closure compilation (it will break internal tests, but not sure why it's not throwing up in tests :)) — since StatusMetadata is not a defined type in Closure JS.

How then can we define it as a type for Closure JS? How is Metadata defined in closure js?

Is this about creating file like this: javascript/net/grpc/web/metadata.js ?

sampajano · 2025-02-22T01:24:30Z

packages/grpc-web/index.d.ts

@@ -1,6 +1,9 @@
 declare module "grpc-web" {

  export interface Metadata { [s: string]: string; }
+  export type StatusMetadata = Metadata & {


I appreciate the type extension, and i personally prefer this kind of change.

However, since I plan to have this feature only in open source grpc-web, i prefer to make minimal changes to the rest of the API (e.g. in RpcError, etc.). Would it be possible if you just modify Metadata instead?

That was my first tought, but, Metadata type is used both for input and output to grpc-services. Also i assume that previous definition od Metadata follows this: https://grpc.io/docs/guides/metadata/#overview (keys are string, values are string or binary).

To not make a breaking change for those who rely on Record<string, string> i created separate Metadata for request statuses (status + error) that additionaly can have new httpStatusCode property of number type.

I think this is more secure way to handle Metadata for status and error.

Don't see any reason why httpStatusCode should be valid property for standard messaging (in-out).

That's why i decided to make separate type.

wszydlak changed the title ~~Enhance HTTP Status Handling in Unary Calls for gRPC-Web~~ Enhance HTTP Status handling in unary calls Jan 20, 2025

sampajano added release notes: yes kokoro:run labels Feb 1, 2025

grpc-kokoro removed the kokoro:run label Feb 1, 2025

sampajano reviewed Feb 1, 2025

View reviewed changes

wszydlak requested a review from sampajano February 1, 2025 19:17

wszydlak changed the title ~~Enhance HTTP Status handling in unary calls~~ Enhance HTTP Status handling Feb 3, 2025

wszydlak force-pushed the feature/http-status-on-error branch 2 times, most recently from 9ef3774 to 82f7f36 Compare February 5, 2025 13:04

sampajano added the kokoro:run label Feb 5, 2025

grpc-kokoro removed the kokoro:run label Feb 5, 2025

feat: pass http status code to RpcError

b84dc9c

wszydlak force-pushed the feature/http-status-on-error branch from 82f7f36 to b84dc9c Compare February 6, 2025 06:41

sampajano added the kokoro:run label Feb 8, 2025

grpc-kokoro removed the kokoro:run label Feb 8, 2025

sampajano reviewed Feb 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance HTTP Status handling #1472

Enhance HTTP Status handling #1472

wszydlak commented Jan 20, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 20, 2025 •

edited

Loading

wszydlak commented Jan 21, 2025

sampajano left a comment •

edited

Loading

wszydlak commented Feb 1, 2025

sampajano commented Feb 3, 2025

wszydlak commented Feb 5, 2025 •

edited

Loading

wszydlak commented Feb 6, 2025

wszydlak commented Feb 11, 2025

sampajano commented Feb 16, 2025

wszydlak commented Feb 20, 2025

sampajano left a comment

sampajano Feb 22, 2025

wszydlak Feb 24, 2025

sampajano Feb 22, 2025

wszydlak Feb 24, 2025

wszydlak Feb 24, 2025 •

edited

Loading

sampajano Feb 22, 2025

wszydlak Feb 24, 2025

	if (xhrStatusCode != -1) {
	errorMessage += ', http status code: ' + xhrStatusCode;
	}

	self.handleError_(new RpcError(grpcStatusCode, errorMessage));

Enhance HTTP Status handling #1472

Are you sure you want to change the base?

Enhance HTTP Status handling #1472

Conversation

wszydlak commented Jan 20, 2025 • edited Loading

Pull Request: Enhance HTTP Status handling in unary calls

Background

Changes introduced in this PR

Alternatives

Example behaviour before and after this change

Before:

After:

Open Questions

Why This Change Is Valuable

linux-foundation-easycla bot commented Jan 20, 2025 • edited Loading

wszydlak commented Jan 21, 2025

sampajano left a comment • edited Loading

Choose a reason for hiding this comment

wszydlak commented Feb 1, 2025

sampajano commented Feb 3, 2025

wszydlak commented Feb 5, 2025 • edited Loading

wszydlak commented Feb 6, 2025

wszydlak commented Feb 11, 2025

sampajano commented Feb 16, 2025

wszydlak commented Feb 20, 2025

sampajano left a comment

Choose a reason for hiding this comment

sampajano Feb 22, 2025

Choose a reason for hiding this comment

wszydlak Feb 24, 2025

Choose a reason for hiding this comment

sampajano Feb 22, 2025

Choose a reason for hiding this comment

wszydlak Feb 24, 2025

Choose a reason for hiding this comment

wszydlak Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

sampajano Feb 22, 2025

Choose a reason for hiding this comment

wszydlak Feb 24, 2025

Choose a reason for hiding this comment

wszydlak commented Jan 20, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 20, 2025 •

edited

Loading

sampajano left a comment •

edited

Loading

wszydlak commented Feb 5, 2025 •

edited

Loading

wszydlak Feb 24, 2025 •

edited

Loading