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(microservices): grpc client closing #12959

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

yurks
Copy link
Contributor

@yurks yurks commented Dec 22, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

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, using GRPC client created through ClientProxyFactory, it's not possible to close once initiated connection.

const client = ClientProxyFactory.create({
      transport: Transport.GRPC
    });

const service = client.getService('ServiceName')

await firstValueFrom(service.callSomething())

// do nothing with connection
client.close()

// workaround to close connection properly
client.getClientByServiceName<Client>('ServiceName').close();

That looks like a bug, as close() method of ClientGrpcProxy class trying to call corresponding method not from Client instance, created by @grpc/grpc-js, but from parsed proto package definition, loaded by @grpc/proto-loader:

  // ! there is packages definitions inside this.grpcClients, not a clients
  public close() {
    this.grpcClients
      .filter(client => client && isFunction(client.close))
      .forEach(client => client.close());
    this.grpcClients = [];
  }

As a result, network connection to GRPC server never ends, once initiated.

Additionally, connections are not gratefully closing on app termination, using GRPC client via ClientsModule, as above described method set for client.onApplicationShutdown hook:

    static assignOnAppShutdownHook(client) {
        client.onApplicationShutdown =
            client.close;
        return client;

Issue Number: N/A

What is the new behavior?

public close() method has been rewritten to properly close the network connection.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented Dec 22, 2023

Pull Request Test Coverage Report for Build c3aa89dd-6768-4bd8-bcb3-9213772bcb6e

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 92.101%

Totals Coverage Status
Change from base Build 7ac098a9-c60b-4faa-9612-e3dbed2e2eb6: 0.001%
Covered Lines: 6716
Relevant Lines: 7292

💛 - Coveralls

.filter(client => client && isFunction(client.close))
.forEach(client => client.close());
this.clients.forEach(
client => client && isFunction(client.close) && client.close(),
Copy link
Member

Choose a reason for hiding this comment

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

any chance we could keep using filter & forEach combination instead of an inline && condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.clients is a map, and there is no .filter
Sure, we can convert map to array here, then filter, then foreach over it, but I found this approach too complicated for this functionality

Copy link
Member

Choose a reason for hiding this comment

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

so instead of using an inline condition could we just use a regular if statement here? (for better readability)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants