Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add docs to explain inbound/outbound #142

Merged
merged 2 commits into from
Mar 19, 2018

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Mar 13, 2018

Resolves #138.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Mar 13, 2018
@Lukasa Lukasa requested review from normanmaurer and weissi March 13, 2018 15:15
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.

great stuff!

/// Despite the fact that `write` is one of the methods on this `protocol`, you should avoid assuming that "outbound" events are to do with
/// writing to sockets. Instead, "outbound" events are events that are passed *to* the socket (or other event source): that is, things you tell
/// the socket to do. That includes `write` ("write this data to the socket"), but it also includes `read` ("please begin attempting to read from
/// the socket") and `bind` ("please bind the following address"), which have nothing to do with sending data.
Copy link
Member

Choose a reason for hiding this comment

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

@Lukasa I am not sure we should use socket here as a Channel may not be backed by a socket.

Copy link
Member

Choose a reason for hiding this comment

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

That said the rest of the docs sounds good... Maybe just make it clear that socket is an example here.

@@ -124,7 +129,12 @@ public protocol _ChannelOutboundHandler: ChannelHandler {

/// Untyped `ChannelHandler` which handles inbound I/O events.
///
/// We _strongly_ advice against implementing this protocol directly. Please implement `ChannelInboundHandler`.
/// Despite the fact that `channelRead` is one of the methods on this `protocol`, you should avoid assuming that "inbound" events are to do with
/// reading from sockets. Instead, "inbound" events are events that originate *from* the socket (or other event source): that is, events that the
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above...

@weissi weissi added this to the 1.2.0 milestone Mar 13, 2018
@Lukasa Lukasa force-pushed the cb-explain-inbound-outbound branch from 1d0c389 to c61d38b Compare March 13, 2018 16:53
@Lukasa
Copy link
Contributor Author

Lukasa commented Mar 13, 2018

Ok, addressed.

@helje5
Copy link
Contributor

helje5 commented Mar 13, 2018

It was me who was hitting this, we talked about it in the Slack. I think those comments are buried too deep and this somehow belongs into the main README or some other document. (Now that I think I understood it, I consider writing a blog entry about it.)

What Inbound and Outbound is, is not obvious to someone who didn't do Netty before (actually I think the naming in general is not particularly good, no offence!)

I (and I suspect many will) originally mapped "Inbound" to the concept of "ReadStream" and "Outbound" to the concept of a "WriteStream". But this is not at all what is going on here. I'm not yet sure how to describe it easily, but the way I understand it Outbound is like a "perform action" and Inbound is like "did perform action". It decouples the emitter from the result handler (in a pretty "distant" way).
(and a thing which makes this harder to understand is autoRead :-) )

@normanmaurer
Copy link
Member

@helje5 I am happy for other naming suggestions. We used ChannelUpstreamHandler and ChannelDownstreamHandler in netty 3 which was even more confusing to people. Honestly most people in netty land like the inbound / outbound naming, but maybe its just because the old names were even worse 🤣

@helje5
Copy link
Contributor

helje5 commented Mar 14, 2018

You are right, I shouldn't complain w/o providing decent alternatives ;->

I'm not sure yet, I don't fully understand yet why you do some things the way you do them. Maybe I come back to you on this later.

For this particular issue, it would be cool to get some more prominent mention of this on the main README or design document (actually I recommend making the README smaller, and put the current stuff in a DESIGN.md or sth).

@Lukasa Lukasa modified the milestones: 1.2.0, 1.3.0 Mar 16, 2018
@weissi
Copy link
Member

weissi commented Mar 19, 2018

@normanmaurer happy with this?

@normanmaurer
Copy link
Member

@weissi yep... missed that @Lukasa did address my comment. Let me merge FTW!

@normanmaurer normanmaurer merged commit a3fad28 into apple:master Mar 19, 2018
weissi pushed a commit to weissi/swift-nio that referenced this pull request Jun 13, 2020
Motivation:

It's important to ensure that child channel users can observe the
writability state of a child channel in order to avoid ballooning data
in memory. Up until now we haven't supported writability notifications,
but we really should.

Modifications:

- Added a flow controller to manage writability.
- Managed data flow through promises.
- Lots of tests.

Result:

Child channels will provide a much better signal as to whether they are
writable.
weissi pushed a commit to weissi/swift-nio that referenced this pull request Feb 3, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants