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

[enhancement] GRPCChannelPool throws error from as deep as the center of the Earth #1459

Open
pacu opened this issue Jul 13, 2022 · 9 comments

Comments

@pacu
Copy link

pacu commented Jul 13, 2022

Is your feature request related to a problem? Please describe it.

Consider this API:

I CMD+clicked my way into the function actually doing the throwing and it seems too far fetched.
throw BoringSSLError.unknownError(errorStack)
image

GRPCChannel.with() ---> try PooledChannel(configuration: configuration) ---> try tlsConfiguration.makeNIOSSLContext() ----> try NIOSSLContext(configuration: configuration.configuration) ---> try self.init(configuration: configuration, callbackManager: nil, additionalCertificateChainVerification: nil)

I'm not sure what I'm supposed to catch, and what the possible errors are, but according to the last breadcrumb, there could be many and also much more deeper.

Describe the solution you'd like

Present a comprehensive list of errors that mask those occurring on dependencies of the package.
A clear and concise description of what you want to happen.

Describe alternatives you've considered

A clear and concise description of any alternative solutions or features you've
considered, if any.

Additional context

Add any other context about the feature request here.
I'm researching this in the process of moving from ClientConnection to GRPCChannelPool. The latter solves many issues my users were experiencing regarding "Transport Unavailable" being thrown #1273 but it makes my service API Throwing and I can't make a sense of it.

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 14, 2022

It's not reasonable to exhaustively list which errors can be thrown since they may be thrown by dependencies. These dependencies can introduce new errors at any time so the goalposts are constantly moving. And, as you've identified, our dependencies can't exhaustively list errors!

@Lukasa
Copy link
Collaborator

Lukasa commented Jul 14, 2022

Concretely, this error is thrown from https://github.com/apple/swift-nio-ssl/blob/c30c680c78c99afdabf84805a83c8745387c4ac7/Sources/NIOSSL/SSLContext.swift#L225. This error is therefore related to the choice of signing algorithms, which would be derived from custom TLS configuration. Are you making any tweaks to the TLS configuration?

@pacu
Copy link
Author

pacu commented Jul 14, 2022

Concretely, this error is thrown from https://github.com/apple/swift-nio-ssl/blob/c30c680c78c99afdabf84805a83c8745387c4ac7/Sources/NIOSSL/SSLContext.swift#L225. This error is therefore related to the choice of signing algorithms, which would be derived from custom TLS configuration. Are you making any tweaks to the TLS configuration?

@Lukasa That's interesting! I'm not actually customizing TLS I'm using whatever is the default for NIOSSL with this transport security configuration

 GRPCChannelPool
            .Configuration
            .TransportSecurity
            .tls(
                .makeClientConfigurationBackedByNIOSSL()
            )

then passing it to the channel constructor

 return try GRPCChannelPool.with(
                target: ConnectionTarget.hostAndPort(host, port),
                transportSecurity: transportSecurity,
                eventLoopGroup: MultiThreadedEventLoopGroup(numberOfThreads: 1)
            )

It would be great if a default config could be non-throwing! 🤩

It's not reasonable to exhaustively list which errors can be thrown since they may be thrown by dependencies. These dependencies can introduce new errors at any time so the goalposts are constantly moving. And, as you've identified, our dependencies can't exhaustively list errors!

Yes, I completely understand your point. Yet I beg to differ that maybe it could be handled in a more developer-friendly, in fact it will probably make troubleshooting easier.

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 14, 2022

It would be great if a default config could be non-throwing! 🤩

Intersting... I cannot reproduce this. What version of NIOSSL are you using?

It's not reasonable to exhaustively list which errors can be thrown since they may be thrown by dependencies. These dependencies can introduce new errors at any time so the goalposts are constantly moving. And, as you've identified, our dependencies can't exhaustively list errors!

Yes, I completely understand your point. Yet I beg to differ that maybe it could be handled in a more developer-friendly, in fact it will probably make troubleshooting easier.

Possibly. I think you end up loosely bucketing errors but to understand the actual root cause you need the original error.

@pacu
Copy link
Author

pacu commented Jul 14, 2022

Intersting... I cannot reproduce this. What version of NIOSSL are you using?

My Package.resolved shows

 {
        "package": "swift-nio-ssl",
        "repositoryURL": "https://github.com/apple/swift-nio-ssl.git",
        "state": {
          "branch": null,
          "revision": "42436a25ff32c390465567f5c089a9a8ce8d7baf",
          "version": "2.20.0"
        }
      },

Possibly. I think you end up loosely bucketing errors but to understand the actual root cause you need the original error.

Yes. I took some time and kept cmd+clicking my way into nio-ssl to know its inner workings. Surveyed the throwing points and opened this issue on swift-nio-ssl apple/swift-nio-ssl#381

It's not that bad but it could be improved so that developers like you and me can provide a better and cleaner API.

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 15, 2022

Intersting... I cannot reproduce this. What version of NIOSSL are you using?

My Package.resolved shows

 {
        "package": "swift-nio-ssl",
        "repositoryURL": "https://github.com/apple/swift-nio-ssl.git",
        "state": {
          "branch": null,
          "revision": "42436a25ff32c390465567f5c089a9a8ce8d7baf",
          "version": "2.20.0"
        }
      },

Hmm, no luck reproducing this with this (with 2.20.0 or other versions). As far as I understand you're essentially doing this:

let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) }

XCTAssertNoThrow(try GRPCChannelPool.with(
    target: .host("127.0.0.1"),
    transportSecurity: .tls(.makeClientConfigurationBackedByNIOSSL()),
    eventLoopGroup: group
))

Do you have a minimal reproducer for this?

Possibly. I think you end up loosely bucketing errors but to understand the actual root cause you need the original error.

Yes. I took some time and kept cmd+clicking my way into nio-ssl to know its inner workings. Surveyed the throwing points and opened this issue on swift-nio-ssl apple/swift-nio-ssl#381

🙏

@pacu
Copy link
Author

pacu commented Jul 15, 2022

Notice that this is a feature request for a less deep throwing API.

I'm not getting any errors but it would be nice to have a friendlier API for it.

I think this bubbles from NIOSSL and there's not much that swift grpc can do than some wrapping. 😥

I'll defer to the issue I created on NIOSSL repo if they improve they throwing the swift grpc will probably improve.

Would it be possible to achieve a "default" setup that could be non throwing ? As @Lukasa implied?

@pacu pacu changed the title GRPCChannelPool throws error from as deep as the center of the Earth [enhancement] GRPCChannelPool throws error from as deep as the center of the Earth Jul 15, 2022
@Lukasa
Copy link
Collaborator

Lukasa commented Jul 15, 2022

To be clear, it’s my understanding that you are currently using a default config. The default config grpc is using should not throw; so your current situation is something we’d like to understand

@pacu
Copy link
Author

pacu commented Jul 15, 2022

I apologize for the misunderstanding. 🙏🙏🙏

I'm not being thrown an error. What I'm describing (and suggesting) is that the throwing originates really deep into the stack so it would be good to have more concise details of what it could cause it to throw an error to reduce the chances of users facing those issues.

I could also add that if there's a known fail-proof default way that most clients will like be using, that a non-throwing API for those cases is provided.

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

No branches or pull requests

3 participants