Skip to content

Made NIOTSEventLoop public #74

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

Closed
wants to merge 1 commit into from
Closed

Conversation

adam-fowler
Copy link
Contributor

[One line description of your change]

I have made NIOTSEventLoop public so I can recognise one and create the correct NIOClientTCPBootstrap for it.

This is required for the PR swift-server/async-http-client#184 which adds support for NIO Transport Services to AsyncHTTPClient. The changes mean I can create the correct NIOClientTCPBootstrap for the eventLoop supplied

Modifications:

Replaced internal for public in front of NIOTSEventLoop

@Lukasa
Copy link
Contributor

Lukasa commented Mar 28, 2020

I am disinclined to make the event loop type public: it should not be necessary. In the same way that SelectableEventLoop is not public, NIO TS should not need a public event loop.

This seems like we need a different solution to this problem.

@adam-fowler
Copy link
Contributor Author

Ok what do you suggest?

AsyncHTTPClient.HTTPClient() is initialised with an EventLoopGroup. I need to recognise when this is either a NIOTSEventLoopGroup or a NIOTSEventLoop. They both require a different NIOClientTCPBootstrap from all other EventLoopGroups.

@Lukasa
Copy link
Contributor

Lukasa commented Mar 29, 2020

In this instance it seems like we’re missing some extra API surface. Probably EventLoopGroup should have a way to hand you a bootstrap. @weissi what do you think?

@Lukasa
Copy link
Contributor

Lukasa commented Mar 29, 2020

Of course the other option is for AsyncHTTPClient to just be told what bootstrap to use.

@adam-fowler
Copy link
Contributor Author

adam-fowler commented Mar 29, 2020

I was just about to suggest the EventLoopGroup.clientTCPBootStrap() -> NIOClientTCPBootstrapProtocol idea

@weissi
Copy link
Member

weissi commented Mar 29, 2020

I tried this at the beginning but you need to match a bootstrap and a TLS implementation that is compatible. I believe the only way to get this to work is to explicitly check which bootstrap you’re handed and then match it with an appropriate TLS provider. I know it’s not nice but there’s little we can do I think.

@adam-fowler
Copy link
Contributor Author

@weissi Aye but I don't even have a bootstrap. I've got an EventLoop. And there currently isn't anyway to get the appropriate bootstrap from an EventLoop (unless I missed it). If we add the function mentioned above, I can do as you suggest above get the bootstrap from the EventLoop, check its type and create the appropriate TLS provider.

@weissi
Copy link
Member

weissi commented Mar 29, 2020

@adam-fowler the only thing you can do unfortunately is to switch over the ELG, if it’s a MultiThreadedELG you need to use ClientBootstrap+NIOSSLTLSProcider; if it’s a NIOTSELG then NIOTSConnectionBootstrap+NIOTSTLS, if anything else: fail :(

I know that’s not nice but somewhere we need to pair the right bootstrap, ELG, and TLS.

@adam-fowler
Copy link
Contributor Author

Yeah but the whole point of this PR was I could be supplied a NIOTSEventLoop which I have no way of recognising.

@Lukasa
Copy link
Contributor

Lukasa commented Mar 30, 2020

@adam-fowler The problem is more foundational than that: there is no reason to assume there are only two event loops in the world. We could easily create NIOIOCPEventLoop, or NIOWin32EventLoop, or any other number of loop abstractions. Indeed, a third does already exist (EmbeddedEventLoop). What we're saying here is that async-http-client needs to stop forgetting which kind of event loop it's operating on by accepting a bootstrap at the same time as it accepts (or creates) its event loop group.

@adam-fowler
Copy link
Contributor Author

@Lukasa I'm quite happy with ditching this PR. But I do need another solution. And as @weissi pointed out the construction of bootstrap with TLS is always going to involve some form of if this bootstrap add this tls. How about?

In swift-nio

protocol EventLoopGroup {
    ...
    func createBootstrap()
}

extension EventLoopGroup {
    func createBootstrap() {
        return ClientBootstrap(group: self)
    }
}

In swift-nio-transport-services

class NIOTSEventLoopGroup {
    func createBootstrap() {
        return NIOTSConnectionBootstrap(group: self)
    }
}    
class NIOTSEventLoop {
    func createBootstrap() {
        return NIOTSConnectionBootstrap(group: self)
    }
}

Then in async-http-client

let bootstrap = eventLoopGroup.createBootstrap()
if bootstrap is NIOTSConnectionBootstrap {
    let tlsProvider = NIOTSClientTLSProvider(tlsOptions: .init())
    bootstrap = NIOClientTCPBootstrap(bootstrap, tls: tlsProvider)
} else if bootstrap is ClientBootstrap {
    ...
} else {
    preconditionFailure()
}

@Lukasa
Copy link
Contributor

Lukasa commented Mar 30, 2020

It remains unclear to me why async-http-client can't just create the bootstrap itself. The client is either handed an EventLoopGroup (which can be interrogated about its type) or is asked to create one itself (in which case async-http-client concretely knows its type). In both cases, you can do the type interrogation at that level.

@adam-fowler
Copy link
Contributor Author

adam-fowler commented Mar 30, 2020

Not if the provided EventLoopGroup is an NIOTSEventLoop.

The protocol doesn’t need to create the Bootstrap it could just return the type of Bootstrap required.

@Lukasa
Copy link
Contributor

Lukasa commented Mar 30, 2020

That's true, though I'll note that the same is true if the EventLoopGroup is a SelectableEventLoop, which also is not public. @weissi seems like we might need to revisit the idea that the ELG can provide a bootstrap.

@weissi
Copy link
Member

weissi commented Mar 30, 2020

@Lukasa That is true. Most of the time, I'd hope the user gives us a MTELG but you're totally right that it could be a selectable one which is private. I don't have a good idea how to fix this because we can't add a requirement to the EL to return us either a typed bootstrap or a NIOClientTCPBootstrap. In either case we'd need to make that optional, therefore also default implement to nil and then we couldn't refine it in NIOSSL :(.

Literally the only thing I can think of is to add some optional initialiser to the concrete bootstraps that say no if they're passed the wrong event loop.

So something like

func makeClientBootstrap(group userProvidedGroup: EventLoopGroup) -> NIOClientTCPBoostrap {
    if let bootstrap = ClientBootstrap(groupOrFail: userProvidedGroup) {
        return NIOClientTCPBootstrap(bootstrap, tls: ...)
    } else if let bootstrap = NIOTSConnectionBootstrap(groupOrFail: userProvidedGroup) { 
       return ....(bootstrap, tls: NIOTSSSL(...))
   } else {
       :(
  }
}

@weissi
Copy link
Member

weissi commented Mar 30, 2020

or maybe

// in the `NIO` module
extension ClientBootstrap {
    public static func canBeUsedWith(_ eventLoop: EventLoop) -> Bool {
        // works here because this is in the NIO module
        return eventLoop is MultiThreadedELG || eventLoop is SelectableEL
   }
}

@weissi
Copy link
Member

weissi commented Mar 30, 2020

The protocol doesn’t need to create the Bootstrap it could just return the type of Bootstrap required.

That of course works too. Not sure how helpful this is though because then you need to switch over the type still...

@adam-fowler
Copy link
Contributor Author

I don't have a good idea how to fix this because we can't add a requirement to the EL to return us either a typed bootstrap or a NIOClientTCPBootstrap.

Not sure why this is the case, but you guys know better. Are there EventLoop's that don't have a associated Bootstrap? I'll let you hammer this one out.

@weissi
Copy link
Member

weissi commented Mar 30, 2020

Not sure why this is the case, but you guys know better. Are there EventLoop's that don't have a associated Bootstrap? I'll let you hammer this one out.

We can't return a typed bootstrap to the requirements because the types are different and we can't return a NIOClientTCPBootstrap because we'd need a TLS provider for that.

@adam-fowler
Copy link
Contributor Author

But you can return a NIOClientTCPBootstrapProtocol

@weissi
Copy link
Member

weissi commented Mar 30, 2020

But you can return a NIOClientTCPBootstrapProtocol

You can't construct a NIOClientTCPBootstrap from a NIOClientTCPBootstrapProtocol existential and that's deliberate because we need a matching TLS implementation.

@adam-fowler
Copy link
Contributor Author

You can't construct a NIOClientTCPBootstrap from a NIOClientTCPBootstrapProtocol existential and that's deliberate because we need a matching TLS implementation.

Ah fair point. How about you have EventLoopGroup just returns the bootstrap type

    func getBootstrapType() -> NIOClientTCPBootstrapProtocol.Type {
        return ClientBootstrap.self
    }

@weissi
Copy link
Member

weissi commented Mar 30, 2020

Ah fair point. How about you have EventLoopGroup just returns the bootstrap type

    func getBootstrapType() -> NIOClientTCPBootstrapProtocol.Type {
        return ClientBootstrap.self
    }

That would work but I don't think it's super helpful because the only thing you can do with that is

switch group.bootstrapType {
    case ClientBootstrap.self:
       return NIOClientTCPBootstrap(ClientBootstrap, tls: ...)
   case NIOTSConnectionBootstrap.self:
      return NIOClientTCPBootstrap(...)
   default:
      // be sad
}

So you still have to write all the awful code and we do forever mandate that one EL(G) has exactly one bootstrap type. That might totally change, therefore I suggested to have a canWorkWith method on the bootstrap that checks if it can work with the provided EL(G).

Ie.

if let bootstrap = ClientBootstrap.canWorkWith(userProvidedELG) {
    return NIOClientTCPBootstrap(...)
} else if let bootstrap = NIOTSConnectionBootstrap.canWorkWith(userProvidedELG) {
    return ...
} else {
   // be sad
}

That does the same but will also work if we ever add another bootstrap that works with MTELG/SelectableEL.

@adam-fowler
Copy link
Contributor Author

ok fair enough

@adam-fowler
Copy link
Contributor Author

I guess we can close this now.

@weissi
Copy link
Member

weissi commented Mar 30, 2020

CC @Lukasa do you agree with the above?

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

I'm requesting changes just so we don't merge by accident

@Lukasa
Copy link
Contributor

Lukasa commented Mar 30, 2020

I agree with the above.

@Lukasa Lukasa closed this Mar 30, 2020
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.

3 participants