-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Introduce implementation for PlainDatagramSocketImpl.mcast_join_leave #1404
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
Conversation
|
Can you come up with a simple test that exercises this code at least partially that we can add under |
|
Sure, maybe I can adapt the original test case. |
|
Done. |
|
This currently fails on Darwin with: |
|
OK I'll check it out. I think I have an idea of what the problem might be. |
|
My local build is still running on my really slow Mac but this should fix the problem. Just out of curiosity - how do you get the tests to run as a native image? |
|
You can use the |
|
Thanks! |
|
Looks like there's one more problem. On it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this comment. Also, can you please add some asserts in the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The success condition is simply that the test completes without error. I can try to think of something else to add though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even something as simple as an Assert.fail() on exception, like we do for GitHub1087 test.
...m/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixJavaNetSubstitutions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to style check error: The value of the local variable ipaddress is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think that might be unused in the upstream code as well. For now I just removed the useless declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...m/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixJavaNetSubstitutions.java
Outdated
Show resolved
Hide resolved
|
It looks as though wrapping with |
|
I wonder why |
|
Yeah I was surprised too. It didn't give me a line number, it happened during pointsto analysis: |
|
Can you try cleaning and rebuilding? I just finished running your latest version (from 3 hours ago) through our internal gate and I don't see the |
|
OK, I'll give it a shot. |
|
I cleaned everything and rebuilt and the error remains. I've pushed up the commits with fixes as previously described, and hopefully you can get the same error I am. |
|
I have the wrong version of eclipse apparently. What are the correct
versions?
…On Thu, Jun 20, 2019 at 8:21 PM Codrut Stancu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixJavaNetSubstitutions.java
<#1404 (comment)>:
> + // 1891 * kernel releases. In the future it's possible that IP_ADD_MEMBERSHIP
+ // 1892 * will be updated to return ENOPROTOOPT if uses with an IPv6 socket (Solaris
+ // 1893 * already does this). Thus to cater for this we first try with the IPv4
+ // 1894 * socket options and if they fail we use the IPv6 socket options. This
+ // 1895 * seems a reasonable failsafe solution.
+ // 1896 */
+ // 1897 static void mcast_join_leave(JNIEnv *env, jobject this,
+ // 1898 jobject iaObj, jobject niObj,
+ // 1899 jboolean join) {
+ static void mcast_join_leave(Target_java_net_PlainDatagramSocketImpl self, InetAddress iaObj, NetworkInterface niObj, boolean join) throws IOException {
+ // The Linux code path uses a different mreq structure, so for type safety we have to factor it out completely.
+ if (IsDefined.__linux__()) {
+ // 1901 jobject fdObj = (*env)->GetObjectField(env, this, pdsi_fdID);
+ final FileDescriptor fdObj = self.fd;
+ // 1902 jint fd;
+ int fd;
You should be able to run with mx gate --strict-mode --tags style
<https://travis-ci.org/oracle/graal/jobs/548381854#L2783>, but you need
to have the right versions of Eclipse JDT, pylint, checkstyle installed.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1404?email_source=notifications&email_token=AAAD4T2IK4EPDQF7GIO34ZLP3QNI7A5CNFSM4HZKURH2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4HEWWQ#discussion_r296058628>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAD4T6JGUCEFURAELPAL2LP3QNI7ANCNFSM4HZKURHQ>
.
|
|
@bobmcwhirter the one that travis downloads. That should be done automatically, I raised this issue with the right people already. |
|
@dmlloyd I run the latest changes in your PR through our internal gate (after fixing the remaining style issues) and all the gates pass. Do you see the error in one of your tests or in the general build? |
|
I'm seeing the error when I run |
|
Maybe you can sanity-check me. Here are the steps I'm running: Am I forgetting something obvious? |
|
I'm running through the steps one more time just to be certain... |
|
Same error. I'm going to update my JDK just to see if that matters. |
|
One other thing to note is that I'm testing with Java 11, not Java 8. |
|
I wonder - the exception occurs in |
|
Pushed up another style fix. |
|
@dmlloyd the steps that you are running look good to me. I don't have access to a Mac to try to debug the issue; maybe @vjovanov can help.
|
|
I don't know what that last CI failure is all about. It looks like |
|
Ignore it, we have an issue for that. |
|
I factored out the method into two, one for Linux and one non-Linux, and that seems to have fixed the constant folding issue. I'll add that as a separate commit so that it can be easily individually reverted if/when the constant folding mystery is solved. |
|
Pushed. |
|
Thank you! I'll pull in the changes. |
Partially fixes #1398