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

Fix #3131: javalib ServerSocket should now be more accepting #3140

Merged

Conversation

LeeTibbert
Copy link
Contributor

@LeeTibbert LeeTibbert commented Feb 5, 2023

Fix #3131

javalib java.net.ServerSocket#accept now accepts new connections without
the cast errors.

Collaboration

User devlam, the author of PR #3131, deserves "co-authored-by" recognition for the
precise localization & reproducing code provided in that PR.

Of course, all bugs remain "authored-by" user LeeTibbert.

Backport

This PR is a candidate for backport to the SN 0.4.x series. I believe/hope it
will backport cleanly. I checked the dates for the IPv6 backport to the SN 0.4.x series and
these changes are relevant to 0.4.x.

Later: I have not been able to get a private baseline (pre-changes of this PR) SN 0.4.x out-of-the-box
Scala 3.2.2, Java 8 build to work after several hours invested. I'll try again if/when my patience recovers.

Given that network code is notorious for being difficult to backport, I will be
trying a private build whilst this PR is in CI.

Later yet: I got a baseline SN 0.4.x out-of-the-box build to work and tried to apply
the` fixes in this PR. Using only the one file with the effective change
caused compilation errors due to differences in 0.4.x.

               I think the best thing for me to do is create a separate 0.4.x PR.
               I already have the minimal change and have tested it.  No IPv6
               on SN 0.4.n, so testing takes less time.  I want to create the
               PR at the beginning, not the end, of a sprint.
Testing:

This PR has been manually tested with both IPv6 & IPv4 connections and passes.
scripted-test java-net-server-socket is intended to exercise accept() but appears
totally non-functional; see Issue #3139.

@LeeTibbert LeeTibbert force-pushed the PR_j_net_ServerSocketAccept_I3131 branch from 86cbea2 to edf474f Compare February 5, 2023 16:41
@LeeTibbert LeeTibbert force-pushed the PR_j_net_ServerSocketAccept_I3131 branch from 8c954db to 2294669 Compare February 5, 2023 16:48
Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

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

Looks good! Don't worry about spurious CI failures involving multithreading / Commix, the fix for them is on its way.

@LeeTibbert
Copy link
Contributor Author

Wojciech, Thank you for the timely review and encouragement.

I was shamelessly punting the multithread errors, figuring that effort is
not complete yet.

I will continue to monitor this CI for what may be "my" errors.

I am at end-of-sprint now but will try a baseline 0.4.x build tomorrow-ish.
If successful, I will then add the changes of this PR. Given all
you went thru, twice, with NetworkInterface.scala, I will feel better
knowing that this backports.

@LeeTibbert
Copy link
Contributor Author

For those who may be looking at this PR in the future:

The effective change is in AbstractPlainSocket.scala, around the inet_mumble() code.

I large number of other changes are an attempt to consistently use the "at1.at(0)". in
the sin6_addr case. "at1" works because it references the same location as "at1.at(0)".
The "at1.at(0)" idiom is longer to read and more typing to write, but it gives a better
correspondence of types: that is, a Ptr[Byte] rather than a Ptr[CStruct1].

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.

Cast exception in ServerSocket on accept().
2 participants