Skip to content

Conversation

@OlegStrokan
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

No needed to change documentation. I didn't update any interface i just add missing code for
grpc-server implementation (made as gprc-client)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently, when I establish a connection between the gRPC client (running on one laptop) and the gRPC server (running on another laptop) and start a bidirectional stream with keepalive enabled, we can successfully detect a disconnection if I turn off the internet on either machine.
The problem is that the detection happens only on the client side—nothing happens on the server side.
It works if I bypass the keepalive settings and use the bare channelOptions like this:

      channelOptions: {
        'grpc.keepalive_time_ms': 30000,
        'grpc.keepalive_timeout_ms': 5000,
        'grpc.keepalive_permit_without_calls': true,
        'grpc.http2.min_ping_interval_without_data_ms': 25000,
        'grpc.http2.max_pings_without_data': 0,
        'grpc.http2.min_time_between_pings_ms': 10000,
      },

reason: We didn't map this values to keepalive object as we do with gprc-client

And this approach works on the server too, but it’s not a convenient solution for users of the library. It should be consistent with what we have for the gRPC client (when we can use keepalive)

Issue Number: N/A

What is the new behavior?

Successfully detection on both of side after time what we defined in keepalive settings:

  keepalive: {
         keepaliveTimeMs: 30000, 
         keepaliveTimeoutMs: 5000, 
         keepalivePermitWithoutCalls: 1, 
         http2MaxPingsWithoutData: 0, 
         http2MinTimeBetweenPingsMs: 10000, 
         http2MinPingIntervalWithoutDataMs: 25000,
   },

Now we have same approach on client and server side

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

FYI: I can push repo which have client + server application and provide info about testing with different machines or using containers with iptables

I tested it locally using both a two-machine setup and a multi-container setup. I updated everything locally, built the library, and injected the server files into my project to make the test 100% accurate.

I can provide additional proof if needed.
Thanks

@coveralls
Copy link

Pull Request Test Coverage Report for Build d8f120f2-38ca-4d61-af98-39b2ad153111

Details

  • 5 of 13 (38.46%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 88.858%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-grpc.ts 5 13 38.46%
Totals Coverage Status
Change from base Build 378a669f-c115-4f29-a313-f5a0e54f01da: -0.08%
Covered Lines: 7329
Relevant Lines: 8248

💛 - Coveralls

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