Bootstraps should be thread-safe/Sendable

[weissi]

NIO's bootstraps have traditionally not been thread-safe (and therefore aren't Sendable either). I'm not so sure anymore if that was historically an accident or deliberate :).

FWIW, Netty's are thread-safe: netty/netty#14063

[Lukasa]

It’s an accident. Their current non-Sendability is a reflection of their actual implementation, but we shouldn’t keep it that way.

[weissi]

It’s an accident. Their current non-Sendability is a reflection of their actual implementation, but we shouldn’t keep it that way.

Okay, sounds good. I'll retitle this issue then.

[natikgadzhi]

Hmm. What's a better approach, make the API backwards-compatible and make the existing bootstraps @unchecked Sendable, or make new Sendable bootstraps that are separate?

I think @unchecked Sendable is the way as this will be much easier on our users?

[Lukasa]

The existing bootstraps absolutely should not be made @unchecked Sendable.

We should be a bit clear here: the bootstraps currently aren't Sendable. This is not a statement about them missing the Sendable conformance, it's that they truly cannot be safely passed across isolation domains. Indeed, I recently fixed a bunch of latent bugs caused by us doing this in #3062.

There's also the matter of the fact that the bootstraps lack of Sendable conformance is caused by the fact that they're classes that use the builder pattern. Most of the NIO team regard this API choice as a mistake, and would prefer the bootstraps to be value-typed structs instead. That would take this current situation, which would require mutexes in order to be made Sendable, and replace it with a situation where the Sendable conformance is trivial.

Finally, the bootstraps would be overdue a replacement anyway, as they're really struggling to handle having NIOAsyncChannel added to their API surface area.

[FranzBusch]

Finally, the bootstraps would be overdue a replacement anyway, as they're really struggling to handle having NIOAsyncChannel added to their API surface area.

To add to this. Any new implementation of bootstraps needs to be closely tied to our new async interop APIs in my opinion. While we need to leave room for just pure NIO based bootstraps. The async interop ones impose additional requirements on the design of bootstraps. Besides becoming Sendable the other important requirement is that they enforce the lifetime of the channel.

When I last thought about this. I was thinking something along this API for new bootstraps:

struct TLSConnection {
  public static func connect<Result, ChannelInitializerResult>(
    target: ConnectTarget,
    channelInitializer: (Channel) -> EventLoopFuture<ChannelInitializerResult >,
    body: (TLSConnection) async throws -> Result
    ) async throws -> Result
}
struct TLSListener {
  public static func listen<Result, ChildChannelInitializerResult >(
    target: ListenerTarget,
    channelInitializer: (Channel) -> EventLoopFuture<Void>,
    childChannelInitializer: (Channel) -> EventLoopFuture< ChildChannelInitializerResult >,
    body: (TLSListener) async throws -> Result
  ) async throws -> Result
}

The above is just meant for illustrative purposes and there a lot of open questions with this.

[natikgadzhi]

There's also the matter of the fact that the bootstraps lack of Sendable conformance is caused by the fact that they're classes that use the builder pattern. Most of the NIO team regard this API choice as a mistake, and would prefer the bootstraps to be value-typed structs instead. That would take this current situation, which would require mutexes in order to be made Sendable, and replace it with a situation where the Sendable conformance is trivial.

This is what I thought — sure, we could attempt to patch up current class-based Bootstraps with NIOLocks or other form of mutexes, but I wasn't sure it's worth it.

I could try and work on that, this is manageable at least in terms of API changes (preferably none), and at least we know where some problems might crop up and test for them.

The shift away from builder pattern to structs is more exciting, but I hear what you're saying @FranzBusch, I won't have enough experience yet to drive the conversation on the right API design and implement them.

[Lukasa]

In the very short term I'm not really sure how valuable the move using mutexes would be. The bootstraps not being Sendable is not something we've received loads of feedback about, but @weissi may have heard more.