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

fix: fix headers not completing when call is terminated #728

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

c-lucera-pvotal
Copy link
Contributor

This PR attempt to solve the issue with headers not being correctly completed after a call is closed as pointed out in #727

To fix the issue I have added in the _terminate() method a check to complete with errors any completer still going (I found _headers and _trailers)

Copy link

linux-foundation-easycla bot commented Aug 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@r-durao-pvotal
Copy link

@kevmoo who would be the right person to take a look at this PR? Thanks in advance!

@mraleph
Copy link
Member

mraleph commented Aug 20, 2024

Please add a regression test and then we can land it.

Copy link

github-actions bot commented Aug 20, 2024

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
grpc None 4.0.0 4.0.1 4.0.0 ✔️
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ✔️
File Coverage
lib/src/client/call.dart 💚 85 %

This check for test coverage is informational (issues shown here will not fail the PR).

API leaks ⚠️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
grpc Any
$1.Duration
ServerHandler

This check can be disabled by tagging the PR with skip-leaking-check.

Package publish validation ✔️
Package Version Status
package:grpc 4.0.1 ready to publish

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@c-lucera-pvotal
Copy link
Contributor Author

c-lucera-pvotal commented Aug 20, 2024

@mraleph
Added the requested test and fixed CI issues like Changelog notes.

However I had to change the previous logic from throwing an error

    if (!_headers.isCompleted) {
      _headers.completeError(GrpcError.unimplemented('Request terminated before headers'));
    }

To completing with an empty value

    if (!_headers.isCompleted) {
      _headers.complete({});
    }

The reason about the change was that the error throwing was really distruptive for the current implementations, instead of receving a error only on active listeners (I.e whoever was actively looking at that completer) all the cancelled calls received 3 throws, one for the cancellation, one for the headers not completed and one for the trailers not completed, this meant that all the tests in this project were failing due to the not catched exceptions, and at the same time, anyone cancelling the call had to deal with 3 different throws, I hope this change make sense to you.

@grpc grpc deleted a comment from github-actions bot Aug 20, 2024
@c-lucera-pvotal
Copy link
Contributor Author

@mraleph saw some failing tests, but those seems unrelated to my changes, any hint about those?

@mosuem
Copy link
Contributor

mosuem commented Aug 22, 2024

These keepalive tests can be flaky, but this doesn't look that way. Let me check.

@c-lucera-pvotal
Copy link
Contributor Author

It looks like that the tearDown somehow is not awaiting the closing before the next setUp is running, maybe they are running in parallel?

in line 52 of keep_alive_test.dart, if you use a different port for each serve you will see the tests not failing anymore

//before tests setup
int port = 8081;

//Line 52
 await server.serve(address: 'localhost', port: port++);

Want me to add this?

@c-lucera-pvotal
Copy link
Contributor Author

Another thing I discovered, if I run only keep alive tests it works out of the box, so probably tests are running in parallel with other tests using the port 8081, another fix is to use the port 8282 on line 52

/Line 52
 await server.serve(address: 'localhost', port: 8082);

@c-lucera-pvotal
Copy link
Contributor Author

c-lucera-pvotal commented Aug 22, 2024

Basically those two tests
https://github.com/grpc/grpc-dart/blob/master/test/keepalive_test.dart
https://github.com/grpc/grpc-dart/blob/master/test/server_cancellation_test.dart

uses port 8081, this is not an issue when run one by one, but running the whole suite with flutter test causes he keep_alive failing because the server cancellation test runs first and occupies the port 8081, I can fix this in my pr by changing the port if you want

@c-lucera-pvotal
Copy link
Contributor Author

Pushed the fix for keepalive_test.dart let me know if you prefer to revert it and solve in a different way

test/keepalive_test.dart Outdated Show resolved Hide resolved
@mraleph mraleph merged commit 4f6fe9b into grpc:master Aug 28, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants