Skip to content
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

Avoid a ClassCastException when channel is NioChildDatagramChannel. #885

Closed
wants to merge 3 commits into from

Conversation

StCostea
Copy link
Contributor

Adding a dummy ReadDispatcher specific for NioChildDatagramChannel. Its dispatch() method is never called.

@StCostea StCostea requested a review from dpwspoon March 15, 2017 15:58
@dpwspoon dpwspoon requested a review from jitsni March 15, 2017 17:11
@dpwspoon
Copy link
Contributor

@jitsni will you review this please

@StCostea
Copy link
Contributor Author

@jitsni Did you have a chance to check this one ? Thanks.

ReadDispatcher readDispatcher = channel instanceof NioSocketChannel
? new TcpReadDispatcher((NioSocketChannel) channel)
: new UdpReadDispatcher((NioDatagramChannel) channel);
ReadDispatcher readDispatcher = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

please format and include brackets on the ifs, this is especially ugly because you indent on the if to where the bracket would be as if the else if is suppose to be under the general if statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The indentation was ok. But there were few tabs in there that messed it up in github. I don't know from where I have those tabs from time to time...


@Override
public boolean dispatch(NioWorker worker, SelectionKey key) {
// return worker.readUdp(key, channel.);
Copy link
Contributor

Choose a reason for hiding this comment

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

why have this comment

@dpwspoon
Copy link
Contributor

@jitsni please review once @StCostea has updated the code

@jitsni
Copy link
Contributor

jitsni commented Apr 3, 2017

Also, it is related to #818

@jitsni
Copy link
Contributor

jitsni commented Apr 3, 2017

The fix avoid the ClassCastException, but I wonder if it masks any actual problem

@@ -629,9 +626,13 @@ public void run() {
int interestOps = channel.getInternalInterestOps();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need the following:
if (channel instanceof NioChildDatagramChannel) {
return;
}
It was there in the previous commits but got removed incorrectly. See the commit that removed it:
432b55c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit you gave did not affect that file. I found the change in commit 105d1c6

If I'm adding this piece of code in register and deregister, it means that I can remove all the changes that I did ? I'll try to check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran some tests, it seems that your suggested change is fixing the issue without requiring the changes I did. I suggest to close this PR without merging and opening a new one only with those changes.

@jitsni
Copy link
Contributor

jitsni commented Apr 4, 2017

Closing this one as a new PR will address the issue.

@jitsni jitsni closed this Apr 4, 2017
@StCostea StCostea deleted the issues/ee_426 branch April 13, 2017 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants