Skip to content

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Oct 30, 2024

Motivation:

The ChannelInvoker protocols are an awkward beast. They aren't really something that people can do generic programming against. Instead, they were designed to do API sharing. Of course, they didn't do that very well, and the strict concurrency checking world has revealed this.

Much of the API surface on ChannelInvoker is confused. There are NIOAnys, which aren't Sendable. We allow sending user events without requiring Sendable. And our two main conforming types are ChannelPipeline and ChannelHandlerContext, two types with wildly differing thread-safety semantics.

This PR aims to clean that up.

Modifications:

  • Deprecated all API surface on ChannelInvoker protocols that uses NIOAny.

    ChannelInvoker has to be assumed to be a cross-thread protocol,
    and that requires that it only use Sendable types. NIOAny isn't,
    so these methods are no longer sound.

  • Re-add non-deprecated versions on ChannelHandlerContext.

    While it's not safe to use the NIOAny methods on Channel or
    ChannelPipeline, it's totally safe to use them on
    ChannelHandlerContext. So we keep those available and
    undeprecated.

  • Provide typed generic replacements on ChannelPipeline and on Channel

    To replace the NIOAny methods on ChannelPipeline and Channel
    we can use some typed generic ones instead. These are not
    defined on ChannelInvoker, as the methods are useless on
    ChannelHandlerContext. This begins the acknowledgement that
    ChannelHandlerContext should not have conformed to these
    protocols at all.

  • Add Sendable constraints to the user event witnesses on ChannelInvoker

    Again, these were missing, but must be there for Channel and
    ChannelPipeline.

  • Provide non-Sendable overloads on ChannelHandlerContext

    ChannelHandlerContext is thread-bound, and so may safely pass
    non-Sendable user events.

Result:

One step closer to strict concurrency cleanliness for NIOCore.

Motivation:

The ChannelInvoker protocols are an awkward beast. They aren't
really something that people can do generic programming against.
Instead, they were designed to do API sharing. Of course, they
didn't do that very well, and the strict concurrency checking
world has revealed this.

Much of the API surface on ChannelInvoker is confused. There are
NIOAnys, which aren't Sendable. We allow sending user events
without requiring Sendable. And our two main conforming types
are ChannelPipeline and ChannelHandlerContext, two types with
wildly differing thread-safety semantics.

This PR aims to clean that up.

Modifications:

- Deprecated all API surface on ChannelInvoker protocols that uses
    NIOAny.

    ChannelInvoker has to be assumed to be a cross-thread protocol,
    and that requires that it only use Sendable types. NIOAny isn't,
    so these methods are no longer sound.

- Re-add non-deprecated versions on ChannelHandlerContext.

    While it's not safe to use the NIOAny methods on Channel or
    ChannelPipeline, it's totally safe to use them on
    ChannelHandlerContext. So we keep those available and
    undeprecated.

- Provide typed generic replacements on ChannelPipeline and
    on Channel

    To replace the NIOAny methods on ChannelPipeline and Channel
    we can use some typed generic ones instead. These are not
    defined on ChannelInvoker, as the methods are useless on
    ChannelHandlerContext. This begins the acknowledgement that
    ChannelHandlerContext should not have conformed to these
    protocols at all.

- Add Sendable constraints to the user event witnesses on
    ChannelInvoker

    Again, these were missing, but must be there for Channel and
    ChannelPipeline.

- Provide non-Sendable overloads on ChannelHandlerContext

    ChannelHandlerContext is thread-bound, and so may safely pass
    non-Sendable user events.

Result:

One step closer to strict concurrency cleanliness for NIOCore.
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Oct 30, 2024
@Lukasa
Copy link
Contributor Author

Lukasa commented Oct 31, 2024

Merging over the API breakage checker because it's wrong about sendability.

@Lukasa Lukasa merged commit de116b7 into apple:main Oct 31, 2024
40 of 43 checks passed
@Lukasa Lukasa deleted the cb-channel-invoker-sendability branch October 31, 2024 11:30
Lukasa added a commit to Lukasa/swift-nio that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants