Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade conformance tests to v1.0.3 #1208

Merged
merged 20 commits into from
Sep 5, 2024
Merged

Conversation

srikrsna-buf
Copy link
Member

@srikrsna-buf srikrsna-buf commented Sep 4, 2024

v1.0.3 of the conformance runner adds cases to cover trailers only responses. The changes to the conformance clients follows the changes done to the reference client.

This also changes the conformance tests for callback clients to handle cancellation tests. For unary we rely on global error handler to fail the process and streaming we rely on no of responses as a check.

fetch on node is not throwing an error on the response reader, instead it silently ends the stream. This resulted in a different error code for cancellation errors. nodejs/undici#1940

Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
@srikrsna-buf srikrsna-buf changed the title Fix gRPC Trailers only response error codes Upgrade conformance tests to v1.0.3 Sep 4, 2024
@srikrsna-buf srikrsna-buf marked this pull request as ready for review September 4, 2024 09:24
@timostamm
Copy link
Member

fetch on node is not throwing an error on the response reader, instead it silently ends the stream.

When a signal passed to fetch is aborted? It would be good to know whether this is expected behavior by the spec, and whether it's a known issue, and is already fixed in one of the more recent Node versions. Can you find out and add a comment in the source?

@srikrsna-buf
Copy link
Member Author

srikrsna-buf commented Sep 4, 2024

@timostamm I couldn't find any issue related to this but was able to pinpoint the timing:

async function run() {
    const controller = new AbortController();
    const res = await fetch("https://example.com/", { signal: controller.signal });
    controller.abort();
    const reader = res.body?.getReader();
    console.log(await reader?.read())
}

run();

This prints { value: undefined, done: true }. If the abort is called after getReader then read throws an error.

Running the same code on chrome throws an AbortError.

@srikrsna-buf
Copy link
Member Author

Sorry, found it: nodejs/undici#1940

Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, but can you split up the changes into separate PRs? At least the fix for gRPC trailers-only responses needs to be in a separate PR so that changes and fixes are understandable from reading the changelog of a release.

packages/connect-web/src/connect-transport.ts Outdated Show resolved Hide resolved
Comment on lines 192 to 200
// Callback clients swallow client triggered cancellations don't report
// that as an error.
if (
err === undefined &&
controller.signal.aborted &&
abortCount == count
) {
error = new ConnectError("operation aborted", Code.Canceled);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this. It's more precise than a known-failing list.

packages/connect-conformance/src/callback-client.ts Outdated Show resolved Hide resolved
packages/connect-conformance/src/callback-client.ts Outdated Show resolved Hide resolved
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
srikrsna-buf and others added 6 commits September 5, 2024 10:10
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Signed-off-by: Timo Stamm <ts@timostamm.de>
Signed-off-by: Timo Stamm <ts@timostamm.de>
Signed-off-by: Sri Krishna Paritala <skrishna@buf.build>
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@srikrsna-buf srikrsna-buf merged commit 32d7ada into main Sep 5, 2024
46 checks passed
@srikrsna-buf srikrsna-buf deleted the sk/conformance_upgrade branch September 5, 2024 15:12
@timostamm timostamm mentioned this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants