Move exceptionCaught(..) and userEventTriggered(..) to ChannelStateHandler

[normanmaurer]

While I was working on the Netty in Action book I notice something I would like to change in our current ChannelHandler hierarchy. So here we go.

I think we should exceptionCaught(..) and userEventTriggered(..) to ChannelStateHandler (currently in ChannelHandler).

The main reasons are:

1. These two callbacks will be called as result of ChannelInboundInvoker.* methods. All others callbacks that are called because of it are already part of ChannelStateHandler. So it is more consistent
2. exceptionCaught(..) should never be called because of an outbound operation. For these the ChannelPromise must be failed if an error happens. So it is missleading to have it part of ChannelHandler as ChannelOperationHandler and ChannelOutboundHandler extend it and so expose it.

[normanmaurer]

@trustin wdyt ?

[jpinner]

w.r.t to moving those from "base" ChannelHandler to the "upstream" ChannelHandler, that sounds find by me since they are "upstream" messages - sry about the netty3 syntax, not going to try to context swap this early in the morning :)

Not sure I agree with the second statement you made though. exceptionCaught(..) can be called because of an outband operation and some Netty handlers rely on this -- WriteTimeoutHandler for example

[trustin]

This leads myself to an interesting idea: how about just merging ChannelStateHandler and ChannelOperationHandler into ChannelHandler? That will make our handler type hierarchy much simpler and also reduce the number of adapter classes.

I wonder how much this will affect the performance. It will make the call stack deeper but it will make pipeline implementation simpler.

[normanmaurer]

Then we could also merge ChannelHandler in... Maybe a good idea!

Sent from my iPhone. Excuse any typos....

Am 28.02.2013 um 18:49 schrieb Trustin Lee notifications@github.com:

This leads myself to an interesting idea: how about just merging ChannelStateHandler and ChannelOperationHandler into ChannelHandler? That will make our handler type hierarchy much simpler and also reduce the number of adapter classes.

I wonder how much this will affect the performance. It will make the call stack deeper but it will make pipeline implementation simpler.

—
Reply to this email directly or view it on GitHub.

[normanmaurer]

@trustin just noticed this is exactly what you said... So deal ?

[trustin]

Deal. Another breakage :-)

[normanmaurer]

yeah... I will take care, so we both are guilty ;)

[trustin]

Just make sure there's no perf regression. :-)

[normanmaurer]

Yeah will test... If it really makes a big difference we can just do what I proposed before

Sent from my iPhone. Excuse any typos....

Am 28.02.2013 um 19:52 schrieb Trustin Lee notifications@github.com:

Just make sure there's no perf regression. :-)

—
Reply to this email directly or view it on GitHub.

[normanmaurer]

@trustin one think I just notice is that it will break our ChannelDuplexHandler as it is not easy to combine two handlers anymore.. maybe we should just go with my first idea.. WDYT ?

[normanmaurer]

Sorry I was talking about CombinedChannelDuplexHandler

[normanmaurer]

@trustin so any preference ? I would like to just move the two methods...

[jpinner]

-1 to merging them

it will break all of the combined encoder/decoder codecs

[normanmaurer]

@jpinner yeah that is why I just want to move the two methods. As otherwise it impossible to combine.

[trustin]

Agreed. #1111 looks fine to me. Let's merge!

[normanmaurer]

merged in.. thanks!

[normanmaurer]

I think we should re-open this and think again about it. After my work on out-of-order events in the pipeline I think again we should move the exceptionCaught(...) method to ChannelStateHandler. This will also make the contract clear in which direction the pipeline is processed. If someone wants to react on exceptionCaught and its own impl is only a ChannelOperationHandler he just need to add another handler in or make his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

[normanmaurer]

So for example the ReadTimeoutHandler would just extend ChannelDuplexHandler.

[trustin]

I see no problem with the current contract? Also, if we move it to ChannelStateHandler, an operation handler that raised an exception mistakenly can never be notified.

Norman Maurer notifications@github.com wrote:

I think we should re-open this and think again about it. After my work
on out-of-order events in the pipeline I think again we should move the
exceptionCaught(...) method to ChannelStateHandler. This will also make
the contract clear in which direction the pipeline is processed. If
someone wants to react on exceptionCaught and its own impl is only a
ChannelOperationHandler he just need to add another handler in or make
his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

[ghost]

s/notified/notified to the operation handler which raised the exception/

Trustin Lee notifications@github.com wrote:

I see no problem with the current contract? Also, if we move it to
ChannelStateHandler, an operation handler that raised an exception
mistakenly can never be notified.

Norman Maurer notifications@github.com wrote:

I think we should re-open this and think again about it. After my work
on out-of-order events in the pipeline I think again we should move
the
exceptionCaught(...) method to ChannelStateHandler. This will also
make
the contract clear in which direction the pipeline is processed. If
someone wants to react on exceptionCaught and its own impl is only a
ChannelOperationHandler he just need to add another handler in or make
his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

[normanmaurer]

I would even say we should remove throw Exception from all methods for a operation handler as it should notify the promise on failure, no?

Am 20.04.2013 um 05:48 schrieb Trustin Lee notifications@github.com:

I see no problem with the current contract? Also, if we move it to ChannelStateHandler, an operation handler that raised an exception mistakenly can never be notified.

Norman Maurer notifications@github.com wrote:

I think we should re-open this and think again about it. After my work
on out-of-order events in the pipeline I think again we should move the
exceptionCaught(...) method to ChannelStateHandler. This will also make
the contract clear in which direction the pipeline is processed. If
someone wants to react on exceptionCaught and its own impl is only a
ChannelOperationHandler he just need to add another handler in or make
his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project
—
Reply to this email directly or view it on GitHub.

[trustin]

That's an interesting idea. I don't think we need to remove 'throws Exception' but just need to catch it in DefaultChannelHandlerContext. However, we can't notify the caught exception if the handler already forwarded the promise to the next handler or the promise has been fulfilled.

Norman Maurer notifications@github.com wrote:

I would even say we should remove throw Exception from all methods for
a operation handler as it should notify the promise on failure, no?

Am 20.04.2013 um 05:48 schrieb Trustin Lee notifications@github.com:

I see no problem with the current contract? Also, if we move it to
ChannelStateHandler, an operation handler that raised an exception
mistakenly can never be notified.

Norman Maurer notifications@github.com wrote:

I think we should re-open this and think again about it. After my
work
on out-of-order events in the pipeline I think again we should move
the
exceptionCaught(...) method to ChannelStateHandler. This will also
make
the contract clear in which direction the pipeline is processed. If
someone wants to react on exceptionCaught and its own impl is only a
ChannelOperationHandler he just need to add another handler in or
make
his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project
—
Reply to this email directly or view it on GitHub.

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

[normanmaurer]

That's true but it just doesn't sound right to allow throw an exception if a promise is passed in. It makes the contract more clear, at least for me ;)

Am 20.04.2013 um 09:31 schrieb Trustin Lee notifications@github.com:

That's an interesting idea. I don't think we need to remove 'throws Exception' but just need to catch it in DefaultChannelHandlerContext. However, we can't notify the caught exception if the handler already forwarded the promise to the next handler or the promise has been fulfilled.

Norman Maurer notifications@github.com wrote:

I would even say we should remove throw Exception from all methods for
a operation handler as it should notify the promise on failure, no?

Am 20.04.2013 um 05:48 schrieb Trustin Lee notifications@github.com:

I see no problem with the current contract? Also, if we move it to
ChannelStateHandler, an operation handler that raised an exception
mistakenly can never be notified.

Norman Maurer notifications@github.com wrote:

I think we should re-open this and think again about it. After my
work
on out-of-order events in the pipeline I think again we should move
the
exceptionCaught(...) method to ChannelStateHandler. This will also
make
the contract clear in which direction the pipeline is processed. If
someone wants to react on exceptionCaught and its own impl is only a
ChannelOperationHandler he just need to add another handler in or
make
his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project
—
Reply to this email directly or view it on GitHub.

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project
—
Reply to this email directly or view it on GitHub.

[normanmaurer]

Also if already notify we can just log the exception and close the channel. As this is most likely a user error / bug.

Am 20.04.2013 um 09:31 schrieb Trustin Lee notifications@github.com:

That's an interesting idea. I don't think we need to remove 'throws Exception' but just need to catch it in DefaultChannelHandlerContext. However, we can't notify the caught exception if the handler already forwarded the promise to the next handler or the promise has been fulfilled.

Norman Maurer notifications@github.com wrote:

I would even say we should remove throw Exception from all methods for
a operation handler as it should notify the promise on failure, no?

Am 20.04.2013 um 05:48 schrieb Trustin Lee notifications@github.com:

I see no problem with the current contract? Also, if we move it to
ChannelStateHandler, an operation handler that raised an exception
mistakenly can never be notified.

Norman Maurer notifications@github.com wrote:

I think we should re-open this and think again about it. After my
work
on out-of-order events in the pipeline I think again we should move
the
exceptionCaught(...) method to ChannelStateHandler. This will also
make
the contract clear in which direction the pipeline is processed. If
someone wants to react on exceptionCaught and its own impl is only a
ChannelOperationHandler he just need to add another handler in or
make
his handler also implement ChannelStateHandler.

@jpinner @trustin WDYT ?

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project
—
Reply to this email directly or view it on GitHub.

---

Reply to this email directly or view it on GitHub:
#1107 (comment)

sent from a mobile device
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project
—
Reply to this email directly or view it on GitHub.

[normanmaurer]

@trustin I think once we know if we should change this or not we can just cut CR2 as I don't think the pipeline lifecycle stuff will need any api changes to get fixed. CR2 is long overdue...

I would change it ;)

[trustin]

As we agreed to disagree, I'm going to close this for now. ;-)

[He-Pin]

@normanmaurer I do think the overdue doesn't matter but the quality matters.