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.

Sign up for GitHub

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

Jump to bottom

fix: keep alive timeout finishes transport instead of connection shutdown #722

Merged
merged 3 commits into from
Sep 6, 2024
Merged

fix: keep alive timeout finishes transport instead of connection shutdown #722

merged 3 commits into from
Sep 6, 2024

Conversation

steffenhaak
Copy link
Contributor

Currently, when the keep alive manager runs into a timeout, the connection is shutdown. The channel, however, cannot recover and stays in a stuck state.

Instead, we should finish the transport, which eventually will change the connection to be in transient failure mode, from where it recovers, once a stable connection can be established again.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jul 3, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@mosuem
Copy link
Contributor

mosuem commented Jul 5, 2024

Thanks for the PR! Sounds good at first glance, but I will check the specs and take a look at this PR next week.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 5, 2024 •
edited
Loading

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
grpc None 4.0.2-wip 4.0.2-wip 4.0.2-wip ✔️
Changelog Entry ✔️
Package Changed Files

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

Coverage ✔️
File Coverage
lib/src/client/http2_connection.dart 💚 79 %

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 $1.Duration
ServerHandler
Any

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

Package publish validation ✔️
Package Version Status
package:grpc 4.0.2-wip WIP (no publish necessary)

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

@grpc grpc deleted a comment from github-actions bot Jul 5, 2024
@steffenhaak
Copy link
Contributor Author

Quote from the specs:

If a keepalive ping is not blocked and is sent on the transport, then the keepalive watchdog timer is started which will close the transport if the ping is not acknowledged before it fires.

Closing the transport should be the appropriate thing to do. But take your time and think about it thoroughly.

I'm using a modified version for now, so there's absolutely no pressure on my side. Please let me know if I can assist you in any way with this.

@mosuem
Copy link
Contributor

mosuem commented Sep 2, 2024 •
edited
Loading

LGTM in general, I filed a PR on your fork to address the failing test. Let me know what you think. Probably has to be slightly rebased after #732 landing.

@mosuem
Copy link
Contributor

mosuem commented Sep 2, 2024

@steffenhaak Also sorry for the delay!

@steffenhaak
Copy link
Contributor Author

@mosuem Great, thank you! I will take a look at it, most likely by the end of this week, and do the rebasing...

@pull-request-size pull-request-size bot added size/M and removed size/XS labels Sep 4, 2024
@steffenhaak steffenhaak force-pushed the fix/keep-alive-timeout-handling branch from b9a7047 to 1c10535 Compare September 4, 2024 14:28
@steffenhaak
Copy link
Contributor Author

@mosuem Your changed test looks good to me. I rebased the PR on the current main branch. So I guess this PR is ready to be merged :)

@mosuem
Copy link
Contributor

mosuem commented Sep 6, 2024

@mosuem Your changed test looks good to me. I rebased the PR on the current main branch. So I guess this PR is ready to be merged :)

It seems to need a changelog entry, but then yes!

@steffenhaak steffenhaak force-pushed the fix/keep-alive-timeout-handling branch from 1c10535 to 97a81b6 Compare September 6, 2024 13:40
@pull-request-size pull-request-size bot added size/S and removed size/M labels Sep 6, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 6, 2024
@steffenhaak
Copy link
Contributor Author

@mosuem done :)

@mosuem mosuem merged commit 071ebc5 into grpc:master Sep 6, 2024
37 checks passed
tsavo-at-pieces added a commit to open-runtime/grpc-dart that referenced this pull request Jan 3, 2025
@tsavo-at-pieces
Squashed commit of the following:
c533913 
commit 471a8b3
Author: Tsavo Knott <tsavo@pieces.app>
Date:   Fri Jan 3 13:02:16 2025 -0500

    Clean Up CI

commit 1632820
Author: Tsavo Knott <tsavo@pieces.app>
Date:   Fri Jan 3 13:00:20 2025 -0500

    True Upstream Master

commit 1dddfe0
Merge: 774bf81 c9d6286
Author: Tsavo Knott <102485237+tsavo-at-pieces@users.noreply.github.com>
Date:   Fri Jan 3 12:32:24 2025 -0500

    Merge pull request #2 from open-runtime/update-from-upstream

    Update from Upstream Master

commit c9d6286
Merge: 774bf81 9a9c017
Author: Tsavo Knott <tsavo@pieces.app>
Date:   Fri Jan 3 12:31:33 2025 -0500

    Merge branch 'upstream-master' into update-from-upstream

commit 774bf81
Merge: 4a043fa 00eabed
Author: Tsavo Knott <102485237+tsavo-at-pieces@users.noreply.github.com>
Date:   Fri Jan 3 12:13:44 2025 -0500

    Merge pull request #1 from open-runtime/aot_monorepo_compat

    Merging AOT Monorepo Compat into our Main Branch

commit 9a9c017
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Dec 17 09:58:22 2024 +0100

    Bump vm_service from 14.3.1 to 15.0.0 (grpc#751)

    Bumps [vm_service](https://github.com/dart-lang/sdk/tree/main/pkg) from 14.3.1 to 15.0.0.
    - [Changelog](https://github.com/dart-lang/sdk/blob/main/CHANGELOG.md)
    - [Commits](https://github.com/dart-lang/sdk/commits/HEAD/pkg)

    ---
    updated-dependencies:
    - dependency-name: vm_service
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
    Co-authored-by: Moritz <mosum@google.com>

commit 3e94fec
Author: Moritz <mosum@google.com>
Date:   Tue Dec 17 09:53:02 2024 +0100

    Update health.yaml (grpc#753)

    * Update health.yaml

    * Upgrade example

    * Fixes

    * try different syntax

    * without endings

    * test new wf

    * new version

    * Works, use main now

    * Add changelog

commit 6676c20
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Dec 16 13:37:35 2024 +0000

    Bump dart-lang/setup-dart from 1.6.5 to 1.7.0 (grpc#746)

commit f61b9a3
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Nov 4 10:45:05 2024 +0000

    Bump actions/checkout from 4.2.0 to 4.2.2 (grpc#744)

commit c063010
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Oct 1 08:51:16 2024 +0000

    Bump actions/checkout from 4.1.7 to 4.2.0 (grpc#741)

commit 04ba68e
Author: Moritz <mosum@google.com>
Date:   Tue Oct 1 04:46:38 2024 -0400

    Rev package:lints (grpc#740)

    * Rev package:lints

    * Add changelog

    * Run CI on 3.5.0

    * Test with 3.2.0

    * Update .github/workflows/dart.yml

    Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>

    * Update .github/workflows/dart.yml

    Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>

    ---------

    Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com>

commit f8bbdce
Author: Kevin Moore <kevmoo@users.noreply.github.com>
Date:   Tue Sep 24 12:07:42 2024 -0700

    ignore unreachable_switch_default in weird switch case (grpc#737)

commit 071ebc5
Author: steffenhaak <haak@aucentiq.com>
Date:   Fri Sep 6 17:13:11 2024 +0200

    fix: keep alive timeout finishes transport instead of connection shutdown (grpc#722)

    * fix: keep alive timeout finishes transport instead of shutting down channel

    * Update keepalive_test.dart

    * Update CHANGELOG.md

    ---------

    Co-authored-by: Moritz <mosum@google.com>

commit 8177633
Author: Moritz <mosum@google.com>
Date:   Fri Sep 6 15:09:54 2024 +0200

    Small fixes (grpc#732)

    * Small fixes

    * Revert changes on file

    * Add changelog

    * Small fixes in keepalive test

    * Add delay

    * Fix symbol visibilty

    * Add try catch for debugging

    * Fail

    * fail

    * Use for loop

commit 38ca626
Author: Lasse R.H. Nielsen <lrn@google.com>
Date:   Mon Sep 2 16:58:43 2024 +0200

    Use `Map.of` instead of `Map.from` in grpc client. (grpc#724)

    * Use `Map.of` instead of `Map.from` in grpc client.

    `Map.of` creates a new map with the same keys, values and *type*
    as the original map, when used without type arguments or context type,
    where `Map.from` creates a `Map<dynamic, dynamic>`.
    (This code failed on an attempt to make `Map.unmodifiable` be more
    strictly typed, like `Map.of` instead of `Map.from`, showing that
    an intermediate map had type `Map<dynamic, dynamic>` unnecessarily).

    Same for using `List.of` instead of `List.from`.

    The new code should be (microscopically) more efficient and type safe,
    and is forwards-compatible with a stronger type on `Map.unmodifiable`.

    (The code can be optimized more. For example
    `List.of(list1)..addAll(list2)` can be just `list1 + list2` or
    `[...list1, ...list2]`, both of which may know the total number
    of elements when doing the initial list allocation.
    This is a minimal change to allow the type changes for `.unmodifiable`
    to get past this very initial blocker in internal tests.)

    * Add changelog and minor version increment.

    And my save removes trailing spaces.

commit 4f6fe9b
Author: c-lucera-pvotal <91328643+c-lucera-pvotal@users.noreply.github.com>
Date:   Wed Aug 28 08:18:15 2024 +0200

    fix: fix headers not completing when call is terminated (grpc#728)

    Fixes grpc#727

commit c18e185
Author: Kevin Moore <kevmoo@users.noreply.github.com>
Date:   Wed Jul 24 14:24:57 2024 -0700

    Fix status badge (grpc#726)

commit b999b64
Author: Galen Warren <galen@cvillewarrens.com>
Date:   Wed Jul 17 08:11:29 2024 -0400

    feat: fix hang that occurs when hot restarting (grpc#718)

commit bf8bbde
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Jul 1 11:56:47 2024 +0000

    Bump dart-lang/setup-dart from 1.6.4 to 1.6.5 (grpc#720)

commit 4aa4c8c
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Jul 1 11:52:08 2024 +0000

    Bump actions/checkout from 4.1.6 to 4.1.7 (grpc#719)

commit dee1b2b
Author: Kevin Moore <kevmoo@users.noreply.github.com>
Date:   Wed May 29 17:23:53 2024 -0700

    Update pubspec.yaml

commit 52023d4
Author: Kevin Moore <kevmoo@google.com>
Date:   Tue May 28 14:47:30 2024 -0700

    code fixes

commit ebb7368
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Wed May 22 06:56:00 2024 +0000

    Bump lints from 3.0.0 to 4.0.0

    Bumps [lints](https://github.com/dart-lang/lints) from 3.0.0 to 4.0.0.
    - [Release notes](https://github.com/dart-lang/lints/releases)
    - [Changelog](https://github.com/dart-lang/lints/blob/main/CHANGELOG.md)
    - [Commits](dart-lang/lints@v3.0.0...v4.0.0)

    ---
    updated-dependencies:
    - dependency-name: lints
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 4e65d4b
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue May 21 11:05:38 2024 +0000

    ---
    updated-dependencies:
    - dependency-name: actions/checkout
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 1495453
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue May 21 11:01:10 2024 +0000

    Bump dart-lang/setup-dart from 1.6.2 to 1.6.4

    Bumps [dart-lang/setup-dart](https://github.com/dart-lang/setup-dart) from 1.6.2 to 1.6.4.
    - [Release notes](https://github.com/dart-lang/setup-dart/releases)
    - [Changelog](https://github.com/dart-lang/setup-dart/blob/main/CHANGELOG.md)
    - [Commits](dart-lang/setup-dart@fedb126...f0ead98)

    ---
    updated-dependencies:
    - dependency-name: dart-lang/setup-dart
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 6586b74
Author: Sarah Zakarias <zarah@google.com>
Date:   Tue May 21 12:30:20 2024 +0200

    Add `topics` to `pubspec.yaml` (grpc#712)

commit 9f65399
Author: Moritz <mosum@google.com>
Date:   Fri May 17 14:53:33 2024 +0200

    Move `codec.dart` to former place (grpc#713)

commit 0d02e43
Author: Moritz <mosum@google.com>
Date:   Mon May 6 06:25:06 2024 -0700

    Remove dependency on `package:archive` (grpc#707)

    * Remove dependency on package:archive

    * Test compression on vm only

    * Add licenses

    * Fix analyze issues

    * Fix codec web

    * Fix licenses

    * Add changelog

commit 078fd23
Author: Moritz <mosum@google.com>
Date:   Thu Apr 25 04:45:40 2024 -0700

    Remove generated `StatusCode` (grpc#703)

    * Remove generated `StatusCode`

    * Rev version for breaking change

    * Upgrade min sdk version

    * Fix issues

commit bdbe5f5
Author: Ruben Garcia <RubenGarcia@users.noreply.github.com>
Date:   Mon Apr 22 16:09:18 2024 +0200

    Fix issue 669 (grpc#693)

    * Fix issue 669

    * Update CHANGELOG.md

    * Update CHANGELOG.md

    * Fix dart format issue.
    Fix prefer single quote issue.

    * Update pubspec and changelog to avoid merge check
    publish / validate
    validate packages

    * Add test for GRPC Compression Flag

    * Fix dart analyze issues.

    * Fix latest dart analyze issue (uninizialized variable)

commit bb8b6e5
Author: Moritz <mosum@google.com>
Date:   Fri Apr 19 02:05:59 2024 -0700

    Make protobuf generated imports absolute (grpc#696)

    * Make protobuf generated imports absolute

    * Stop test for now

commit b05fafe
Author: Moritz <mosum@google.com>
Date:   Mon Apr 15 04:43:26 2024 -0700

    Add Health workflow (grpc#699)

    * Add Health workflow

    * Remove license check

commit aece2a4
Author: Abdul Momin <98901875+Curious-x@users.noreply.github.com>
Date:   Mon Apr 15 12:53:00 2024 +0500

    Typo Correction in README.md (grpc#695)

    Corrected typo "RPs" to "RPCs". To avoid confusion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@steffenhaak @mosuem