feat: expose invalid argument error to clients in bidirectional streaming (#4795) #4819

ianbbqzy · 2024-10-10T07:12:07Z

References to other Issues or PRs

Fixes #4795

Have you read the Contributing Guidelines?

yes

Brief description of what is fixed or changed

For bidirectional streaming, allows request decoder to alert the clients of invalid requests.

Other comments

…ming (grpc-ecosystem#4795)

johanbrandhorst

Looks like there's a Bazel failure and a test failure

examples/internal/proto/examplepb/stream.pb.gw.go

ianbbqzy · 2024-10-15T18:22:10Z

Hi @johanbrandhorst , I'm having some trouble running the integration tests following the instructions. I'm not really familiar with gulp and having issue trying to run the gulp CLI commands

ianlee@Ians-MacBook-Pro-2 browser % gulp serve
AssertionError [ERR_ASSERTION]: Task function must be specified
    at Gulp.set [as _setTask] (/Users/ianlee/grpc-gateway/examples/internal/browser/node_modules/undertaker/lib/set-task.js:10:3)
    at Gulp.task (/Users/ianlee/grpc-gateway/examples/internal/browser/node_modules/undertaker/lib/task.js:13:8)
    at Object.<anonymous> (/Users/ianlee/grpc-gateway/examples/internal/browser/gulpfile.js:26:6)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
    at requireOrImport (/usr/local/lib/node_modules/gulp-cli/lib/shared/require-or-import.js:20:11)
    at execute (/usr/local/lib/node_modules/gulp-cli/lib/versioned/^5.0.0/index.js:35:3)
    at onExecute (/usr/local/lib/node_modules/gulp-cli/index.js:230:24)
    at Liftoff.<anonymous> (/usr/local/lib/node_modules/gulp-cli/index.js:153:5)
    at Liftoff.execute (/usr/local/lib/node_modules/gulp-cli/node_modules/liftoff/index.js:320:14)
    at module.exports (/usr/local/lib/node_modules/gulp-cli/node_modules/flagged-respawn/index.js:52:3)
    at Liftoff.<anonymous> (/usr/local/lib/node_modules/gulp-cli/node_modules/liftoff/index.js:310:7) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

would you be able to help?

johanbrandhorst · 2024-10-15T18:41:02Z

I'm... also not much of an expert unfortunately. I usually just let CI run the browser test suite 😬. It looks like the node tests passed in CI on the latest push.

johanbrandhorst

Thanks for the updates, some more questions

examples/internal/proto/examplepb/stream.pb.gw.go

ianbbqzy · 2024-10-17T18:41:18Z

do you know if anyone is familiar and could help with setting up the integration tests?

(I think the problem is with gulp version. will try again on my own as well)

johanbrandhorst · 2024-10-17T21:15:57Z

It might be easier to run if you use our CI environment docker image as described in CONTRIBUTING.md, but I'm also happy to just trigger the CI run for you.

ianbbqzy · 2024-10-17T22:17:22Z

I managed to get gulp to work by forcing it to use version 3.9.1 and also forcing the use of node v10. but don't think it helps with running the integration tests.

if i just use the docker command like

docker run -v $(pwd):/grpc-gateway -w /grpc-gateway --rm ghcr.io/grpc-ecosystem/grpc-gateway/build-env:1.23 \
    /bin/bash -c 'make install && \
        make clean && \
        make generate && make test'

I get similar errors like 2024/10/17 22:11:19 ERROR: http error 502 request body as in the github CI environment
along with errors like

--- FAIL: TestNonStandardNames (20.04s)
    --- FAIL: TestNonStandardNames/Test_standard_update_method (0.02s)
        integration_test.go:2405: {"code":14,"message":"name resolver error: produced zero addresses","details":[]}
        integration_test.go:2408: patchResp.StatusCode= 503; want 200 resp: {"code":14,"message":"name resolver error: produced zero addresses","details":[]}
        integration_test.go:2414: marshaler.Unmarshal failed: proto: (line 1:2): unknown field "code"

note that I'm running this on the main branch so it's not related to my changes in this PR, which would've run into the same issues

johanbrandhorst · 2024-10-18T04:25:02Z

Please ignore the gulp tests. It looks like there might be a problem with them, but I don't think this PR will change anything. As you can see, the go test ./... CI job is failing right now, which is more concerning. Looks like TestABE failed to run:

panic: test timed out after 10m0s
running tests:
	TestABE (9m40s)

ianbbqzy · 2024-10-19T00:30:26Z

Sorry if I'm misunderstanding the testing structure, but from my perspective, TestABE is part of the integration tests, which I tried to run in the docker command with Make test which in turns calls go test -race ./examples/internal/integration -args -network=unix -endpoint=test.sock.

So I would like to debug the test that's failing, but it would be hard without being able to actually run the integration tests properly from my machine.

johanbrandhorst · 2024-10-19T00:34:26Z

All that sounds right, but that doesn't actually require running gulp, does it? On main, I can run this test locally with:

$ go test ./examples/internal/integration/ -run '^TestABE$'
ok      github.com/grpc-ecosystem/grpc-gateway/v2/examples/internal/integration 10.093s

Does that work for you?

johanbrandhorst · 2024-10-21T17:42:45Z

examples/internal/proto/examplepb/stream.pb.gw.go

+		go func() {
+			for err := range reqErrChan {
+				if err != nil && err != io.EOF {
+					runtime.HTTPError(annotatedContext, mux, outboundMarshaler, w, req, err)


Is it safe to call this function with the shared w? Is there some way we can avoid doing this concurrently (a defer? or something).

so testing this implementation with my server works fine but I agree there might be more optimal solution.

I'm not sure about the concern around sharing w, the timing of this invalid argument error and the timing of a stream of valid responses could happen concurrently. I don't see any better indicators for when it's appropriate to send the error the client.

But one thing I noticed is that runtime.HTTPError modifies the header, which means the header could be modified twice. I'm thinking about writing the error directly using w.Write(). My thinking is that if the error occurs in the middle of the stream, it makes sense that the original response header stays at 200.

I have been trying to modify the integration test to test it out but I found that the current test set up waits for the entire request body to be written before actually making the http request. I would expect the response collection to start without waiting for the first goroutine of requests writing to complete. Unfortunately I haven't figured out why that isn't the case yet so it's hard to test the scenario of error occurring in the middle of a stream.

It's fine if we can't easily test it. I don't think setting the header will matter once we've started writing to the body, since we can't write the header again. Given that, this is probably fine :). Thanks for testing it manually.

Hey Johan, thanks for approving this, but I still find that using ErrorHandler might not be ideal. PTAL at #4864. Let's move the discussion over as well

ianbbqzy · 2024-10-22T05:57:25Z

BTW, the vscode dev containers worked like a charm. Shouldn't have fixated on the docker commands and I assumed it would be the same issues with the dev containers.

feat: expose invalid argument error to clients in bidirectional strea…

19cf134

…ming (grpc-ecosystem#4795)

johanbrandhorst requested changes Oct 12, 2024

View reviewed changes

examples/internal/proto/examplepb/stream.pb.gw.go Outdated Show resolved Hide resolved

examples/internal/proto/examplepb/stream.pb.gw.go Show resolved Hide resolved

Ian Lee added 2 commits October 15, 2024 17:00

fix for loop

8015368

remove for loop

690ed31

ianbbqzy requested a review from johanbrandhorst October 16, 2024 00:07

johanbrandhorst requested changes Oct 17, 2024

View reviewed changes

examples/internal/proto/examplepb/stream.pb.gw.go Outdated Show resolved Hide resolved

examples/internal/proto/examplepb/stream.pb.gw.go Outdated Show resolved Hide resolved

revert to for loop

8184dce

ianbbqzy requested a review from johanbrandhorst October 17, 2024 18:37

move stream close to not block stream.header()

a999306

johanbrandhorst reviewed Oct 21, 2024

View reviewed changes

johanbrandhorst approved these changes Oct 23, 2024

View reviewed changes

johanbrandhorst merged commit 830ba27 into grpc-ecosystem:main Oct 23, 2024
17 checks passed

ianbbqzy mentioned this pull request Oct 23, 2024

feat: use StreamErrorHandler to send back invalid argument error in bidirectional streaming #4864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose invalid argument error to clients in bidirectional streaming (#4795) #4819

feat: expose invalid argument error to clients in bidirectional streaming (#4795) #4819

ianbbqzy commented Oct 10, 2024

johanbrandhorst left a comment

ianbbqzy commented Oct 15, 2024

johanbrandhorst commented Oct 15, 2024

johanbrandhorst left a comment

ianbbqzy commented Oct 17, 2024 •

edited

Loading

johanbrandhorst commented Oct 17, 2024

ianbbqzy commented Oct 17, 2024

johanbrandhorst commented Oct 18, 2024 •

edited

Loading

ianbbqzy commented Oct 19, 2024

johanbrandhorst commented Oct 19, 2024 •

edited

Loading

johanbrandhorst Oct 21, 2024

ianbbqzy Oct 22, 2024 •

edited

Loading

johanbrandhorst Oct 23, 2024

ianbbqzy Oct 23, 2024

ianbbqzy commented Oct 22, 2024

feat: expose invalid argument error to clients in bidirectional streaming (#4795) #4819

feat: expose invalid argument error to clients in bidirectional streaming (#4795) #4819

Conversation

ianbbqzy commented Oct 10, 2024

References to other Issues or PRs

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

Other comments

johanbrandhorst left a comment

Choose a reason for hiding this comment

ianbbqzy commented Oct 15, 2024

johanbrandhorst commented Oct 15, 2024

johanbrandhorst left a comment

Choose a reason for hiding this comment

ianbbqzy commented Oct 17, 2024 • edited Loading

johanbrandhorst commented Oct 17, 2024

ianbbqzy commented Oct 17, 2024

johanbrandhorst commented Oct 18, 2024 • edited Loading

ianbbqzy commented Oct 19, 2024

johanbrandhorst commented Oct 19, 2024 • edited Loading

johanbrandhorst Oct 21, 2024

Choose a reason for hiding this comment

ianbbqzy Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

johanbrandhorst Oct 23, 2024

Choose a reason for hiding this comment

ianbbqzy Oct 23, 2024

Choose a reason for hiding this comment

ianbbqzy commented Oct 22, 2024

ianbbqzy commented Oct 17, 2024 •

edited

Loading

johanbrandhorst commented Oct 18, 2024 •

edited

Loading

johanbrandhorst commented Oct 19, 2024 •

edited

Loading

ianbbqzy Oct 22, 2024 •

edited

Loading