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

Make removeHandlers public. #408

Merged
merged 1 commit into from May 18, 2018

Conversation

Projects
None yet
3 participants
@Lukasa
Copy link
Contributor

Lukasa commented May 16, 2018

Motivation:

If ChannelPipeline.removeHandlers is internal it is extremely difficult
to implement a correct ChannelCore.

Modifications:

Make ChannelPipeline.removeHandlers public.

Result:

Easier to implement ChannelCore.

@Lukasa Lukasa added this to the 1.7.0 milestone May 16, 2018

@Lukasa Lukasa requested review from normanmaurer and weissi May 16, 2018

///
/// This method must only be called from within the `EventLoop`. It should only be called from a `ChannelCore`
/// implementation. Once called, the `ChannelPipeline` is no longer active and cannot be used again.
public func removeHandlers() {

This comment has been minimized.

@weissi

weissi May 16, 2018

Contributor

I think we should hide this method better, maybe on the ChannelCore and export it through:

self._unsafe.removeHandlers(self.pipeline)

This comment has been minimized.

@Lukasa

Lukasa May 16, 2018

Author Contributor

This actually can't be on the ChannelCore because the ChannelCore doesn't own the pipeline, the Channel does. Not really sure about exactly what to do there: suggestions?

This comment has been minimized.

@normanmaurer

normanmaurer May 17, 2018

Member

hmm good question. If we make it part of the ChannelPipeline we should at least underscore it imho.

This comment has been minimized.

@weissi

weissi May 17, 2018

Contributor

@Lukasa why not pass the pipeline into channel core like in my example?

This comment has been minimized.

@Lukasa

Lukasa May 17, 2018

Author Contributor

@weissi Mostly because close0 is a method on ChannelCore, not Channel.

As a broader statement I think this might be reflective of a layering issue here. We have defined ChannelCore and Channel as separate protocols without a requirement that they be implemented by the same entity. Channel has a close method but it's explicitly defined as firing down the ChannelPipeline, which will eventually end up invoking ChannelCore.close0. The unfortunately irony here is that ChannelCore.close0 cannot actually close the pipeline, because the ChannelCore does not know where the pipeline is unless the same object implements both ChannelCore and Channel.

In practice all currently written channel implementations of which I am aware do implement both Channel and ChannelCore directly. If we think that's a sensible design pattern then we should enforce that requirement going forward. Otherwise, we should decide how this layering is supposed to work with regard to closing pipelines.

One proposal is to say that, while Channel and ChannelCore may be implemented by different objects, they naturally must have some kind of API between them, because closeFuture (another object required for correctly signalling closure) is also on Channel and not ChannelCore. That means it's simply not possible to write a fully-functional channel without communicating between the two entities. In that circumstance I think the best place to put this extension is actually on Channel, and we should define this:

public extension Channel {
    public func _closedFromChannelCore(error: Error?) {
        self.pipeline.removeHandlers()
        if let error = error {
            self.closeFuture.fail(error: error)
        } else {
            self.closeFuture.succeed(result: ())
        }
    }
}

This comment has been minimized.

@weissi

weissi May 17, 2018

Contributor

not a huge fan of having this public on the Channel.

This comment has been minimized.

@weissi

weissi May 17, 2018

Contributor

still prefer doing the layering violation and doing this:

public extension ChannelCore {
    public func removeHandlers(channel: Channel) {
        channel.pipeline.removeHandlers()
    }
}

This comment has been minimized.

@Lukasa

Lukasa May 17, 2018

Author Contributor

Yeah, I think that's the right thing to do right now. @weissi and I discussed this and we think that actually, ChannelCore and Channel have an implicit expectation that they are implemented on the same object: the fact that they're separate protocols is only to prevent users handling the Channel existential from seeing the methods of ChannelCore all the time.

Longer term we may want a better solution, but for now I think it's fine to just do the layering violation. I'll leave a comment to that effect.

This comment has been minimized.

@Lukasa

Lukasa May 17, 2018

Author Contributor

Updated.

This comment has been minimized.

@normanmaurer

normanmaurer May 17, 2018

Member

works for me.

@Lukasa Lukasa force-pushed the Lukasa:cb-make-remove-handlers-public branch from 5ea1458 to 0a0470d May 17, 2018

@weissi

weissi approved these changes May 17, 2018

Copy link
Contributor

weissi left a comment

🙌

Add ChannelCore.removeHandlers.
Motivation:

If ChannelPipeline.removeHandlers is internal it is extremely difficult
to implement a correct ChannelCore. For that reason, we should make it
available as an extension on ChannelCore.

This is a minor layering violation, but that's ok: ChannelCore and
Channel are not actually intended to be separate objects in most cases,
they are just separate for code clarity reasons.

Modifications:

Add ChannelCore.removeHandlers as a public method.

Result:

Easier to implement ChannelCore.

@Lukasa Lukasa force-pushed the Lukasa:cb-make-remove-handlers-public branch from 0a0470d to f945ce6 May 17, 2018

@normanmaurer normanmaurer merged commit bad7c29 into apple:master May 18, 2018

2 checks passed

pull request validation (4.0.3) Build finished.
Details
pull request validation (4.1) Build finished.
Details

@Lukasa Lukasa deleted the Lukasa:cb-make-remove-handlers-public branch May 18, 2018

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