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

THRIFT-4251 Fix JDK Epoll Bug in Thrift of TThreadedSelectorServer model. #1313

Closed
wants to merge 1 commit into from

Conversation

Johnny-Liao
Copy link

Such as the title. More information see : https://issues.apache.org/jira/browse/THRIFT-4251

@jeking3
Copy link
Contributor

jeking3 commented Aug 13, 2017

According to the documentation of the defect that triggered this (http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6670302), it indicates this only occurs in kernel 2.4 series, and if you move to kernel 2.6 this issue goes away. Given this is an operating system defect, I'm inclined to recommend we do not merge this and resolve it as won't fix, because it isn't a defect in Thrift or in Java, but rather in the operating system it is running on.

@Johnny-Liao
Copy link
Author

My linux kernel is 3.10.0 when this bug occured. And I'm pretty sure that still trouble many people who dependence Thrift to implements theirs server program. Netty uses this approach to mask the underlying problem and why can't we.

@jeking3
Copy link
Contributor

jeking3 commented Aug 13, 2017

There are conflicting reports to what the root cause of this issue really is. I found:

http://bugs.java.com/view_bug.do?bug_id=6693490 fixed in Java 7b55
http://bugs.java.com/view_bug.do?bug_id=6897993 possibly caused by above, fixed in Java 7b77
http://bugs.java.com/view_bug.do?bug_id=6670302 bug in linux 2.4 kernel fixed in 2.6

Given you reproduced it under Java 8b131 then it sounds like it would be a different issue to what's already been reported, or a regression in one of them. Folks seem to think this issue was resolved. I actually found a number of projects that implemented their own solution, but they were all slightly different.

Have you looked into reopening 6670302 with a sample program that recreates it under Java 8b131? Or, does the sample app used in that defect reproduce on your system possibly?

Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

I reviewed the change and it looks okay to me. I'm just questioning where the root cause is and why it isn't an open bug in a dependent project (JDK, OS). I'm going to be away for a couple weeks so someone else will need to pick this up. Thanks.

@Johnny-Liao
Copy link
Author

I found a report maybe more relevant with this issue.
See : http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6403933
It gives a solution what is creates a new Selector to replace the old one.

@jeking3
Copy link
Contributor

jeking3 commented Aug 13, 2017

However, as with all the other issues, they were already resolved, so the fact you are still seeing this with a recent kernel and java version means there's still a bug that needs to be fixed somewhere. There shouldn't be a workaround in every Java program that uses a Selector(). I would still encourage you to come up with a simple example that demonstrates it without Thrift involved and report it upstream.

@Johnny-Liao
Copy link
Author

It's not elegant way to rebuild Selector in every Java program, but it's a effective way to fix this bug. And so many year passed it not really fixed in JDK or kernel. This is one reason for some Frameworks implemented this solution. After all, the consequences of this bug are too serious.

@xiaohu-zhang
Copy link

http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6403933
it sounds like have been resolved yet in jdk7

@jeking3
Copy link
Contributor

jeking3 commented Sep 7, 2017

I'd like additional feedback from folks who use Thrift with Java on this. It seems like the wrong place to fix the issue, but it sounds like many projects are implementing their own fix for it. I've never seen it happen.

@jeking3
Copy link
Contributor

jeking3 commented Sep 21, 2017

So I asked on the mailing list and it's been crickets, so I'm going to merge this. It passed all the tests.

@asfgit asfgit closed this in 9ffb41d Sep 21, 2017
@xiaohu-zhang
Copy link

MY english is poor. could you please explain what is the it's been crickets mean ? thank you very much!

@jeking3
Copy link
Contributor

jeking3 commented Sep 22, 2017 via email

gadLinux pushed a commit to gadLinux/thrift that referenced this pull request Sep 22, 2017
@xiaohu-zhang
Copy link

Thank you ! honestly speaking , netty and lots of framework implementing this fix. I also doubt
It is useful in jdk7. I agree with you ,But if I was you , I will not change the original code,unless give
the unit test to prove it happend in jdk7

jeking3 pushed a commit to jeking3/thrift that referenced this pull request Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants