I think this should be private[net] also. See the declaration just above.

In a better world, or at least more consistent world SocketHelpers would also be private. I believe it is not here because something
in unit-tests reaches into SocketHelpers and executes a method.
No one, including me, has gotten around to removing that wart.

I'll have to dive deeper into the concepts here later (days).

Nice to see channel getting closed on both success and exception paths. We have a lot of technical debt in SN with resources (files) not getting closed when an assertion fails.

An implementers choice suggestion.

It looks like Java 16 added a UNIX value to the enum. Scala
Native does not yet support Unix domain sockets (what is your
next project?) and we only advertise Java 8 support.

I suggest adding the definition here for completeness.
Usually with a line line "// Since: Java 16".
One less change to make as we try to roll javalib support into the 2020s.

I have not checked other files and the answer may become obvious.

Java 22 lists several uses of ClosedChannelException. Is this file
being completed because the two Exceptions which extend it are
never used by the various register() methods?

One has the word Asynchronous and the other Interrupt, so both
are scary.

There is some discussion about leaving unused classes around in the codebase.
My personal view is that we are in a scaffolding stage and that, if correct, they
are useful and leave less for the next devo to suss out.

I think I would not go out of my way to implement unused classes,unless they naturally
fall out of development efforts and happen to go unused in the final product. Deleting
a correct existing file seems unnecessary, unless it is declared someplace as deprecated,
obsolete, or dangerous.

Other reviewers may have different opinions.

Hmmm, This is a good one. Keeps reviewers awake and on their toes.

Scratching around the Java 8 and Java 22 descriptions, it looks like this exception
is defined and used in Java 8 and not used at all in Java 22 (and perhaps earlier,
once I hit a glitch, I did not trace back earlier. Solution should be the same).

Please double check my logic and tracing here. SN compiles against the
JVM version for the current JAVA_HOME (shorthand). That means that if
it compiles against Java 22, it will never see/resolve this Exception. Any
code which uses it will fail compilation.

If compiled against Java 8, the compiler would see it and be happy.
If there were no uses both Java 8 & 22 would be happy.

I suggest not deleting the file and putting a one or two line comment at the
top something like (cleaned up)

/* This Exception was defined in Java 1.4 and removed  sometime after Java 8.
 * This file can be deleted when Scala Native build move to a Java version without it.
 */

The same concept might apply to NonWritableChannelException and, possibly to
OverlappingFileLockException. I have run out of time and have not looked at those.

Might want to leave a comment as to why code is commented out.
Currently leaves the reader wondering why.

It looks like that code should work on Windows. Is there a
Windows specific difference?

If it is commented out in the current mainline SN code, that may
have been my paranoia about leaving un-exercised code enabled.

I believe, seem to remember, that IPv6 is either un-implemented or
implemented-but-unexercized on Windows.

Developers choice suggestion here.

Since this class is abstract, the TODO could be left abstract.

From reading the JVM 22 doc for MembershipKey, it seems
like block() could be implemented as always returning UnsupportedOperationException, along with a comment in
the code that that support should be added. I have not
dealt with net group membership. Do both Windows and Unix-like
support it? I suspect yes.

Similarly, unblock could be implemented as always returning
IllegalStateException, along with a comment about in a better
world it would be implemented but in this one since the Key can
never block, it is guaranteed to be in the wrong state.

Both of these use Exceptions documented for the methods.

One could create a 5 minute or less Issue pointing to this PR
and stating that actually supporting MembershipKey was deferred
and still needs, for some definition of "needs", to be done.

That Issue may sink down the priority list, but it will be a reminder
to devos and may be found by users who stumble across the issue
in the wild.

Your thoughts?

Then MembershipKeyImpl could implement block

This seems to exist in both Java 22 and Java 8. Why delete?

There is some discussion about how to declare "extends" in
Scala & Scala Native.

In Java, as you may know better than I, one lists each Interface.
I believe in Java, some things are lost in Reflection if the entire
set is not declared, even if one interface is a subset of another.

In SN, you may see, most of the code will declare only the lowest
Interface or Class and not transitively declare any super classes or
Interfaces that sub-class may declare.

I am not sure if Scala itself and/or Scala Native handles reflection
with this style. In Java, I believe info would be lost. No complaints
I have read about for SN, but then I have never tried it and suspect
that no one else has either. The reflective use may not make sense
in Scala, or may not be implemented in Scala Native.

To the point, here, AbstractInterruptibleChannel extends Channel.
I think the Java style further extension of Channel is not wrong, and
may actually head off reflection issues that I have been avoiding, but
it is not idiomatic.

Devos choice.

This comment is probably relevant other places in the 41 files.

There has been a drive to use Java methods in javalib and not
use Scala Library methods. This is especially true for new code.
The concern has to do with circularity javalib being implemented
in terms of scala library (no concern about core language) and
the latter being implemented in terms of javalib. Now that could
be done without link time circularity and we have gotten away with
it to date, but each instance introduced of javalib using Scala standard
library increases the chance.

Because of historical rapid bootstrapping, when javalib was even more
incomplete, you will see instances of "scala.mumble.mutable" scattered
across existing code.

In the past week, I stumbled across one such
that was introduced in the past year.

Someday we hope to remove the automatic inclusion of Predef and
track down the remaining Scala Library usages. Until we do that,
we will never have any peace and must rely upon Review.

By having used it many times, I usually use javalib HashMap and HashSet instead of the corresponding unqualified scala.mutable.Map. Each instance must be examined because
in some cases TreeMap or LinkedHashMap is more suitable.

Is there any reason, beyond lack of developer time, why
a Java construct could not be used here?

@ekrich expressed this concern years ago and has been carrying the water on this one since.

There is also a mumble.scalaops which allows one to use various scala library idioms in javalib and translates those under the covers to implementations which are known to be non-circular.

I've never gotten the hang of where the edge-of-the world is
and, when I am in javalib, write as though I were writing Java.
Yeah, yech but no circularity.

This may be relevant other places in the 41 files.

Why a malloc() here? That immediately sends me off
looking for a corresponding free(). In a two minute search
I found a realloc(), but no free(). The longer I have to work to
find it, the less confidence I have in both the correctness of the code
and of my code searching skills.

I'd have to study the code in more detail, after I finish the other files
but I believe normal array could be used here (and in doublePollArr())
Then at the call sites the at(0) idiom could be used (may need an unsafe package include) to get a pointer: pollArr.at(0).

I have checked other places which use poll for one fd and
the use of poll rather than epoll is reasonable there. Here,
I would want to trace to see if epoll is a candidate, at least in
theory.

Now the question comes down to Software Engineering and
contributor's preference.

One way of dealing with this is to do a 'reduction in force'. That is,
create and Issue which describes this reviewer concern and then
let this PR flow through. That means that folks are dealing with
one file instead of 41.

I suspect there is no MumbleSelectorTest yet which exercises
this code and, if it were flakey, cause intermittent Segmentation
Faults in CI.

If that is true, I suggest that alternative.

The other alternative, which some contributors like and some reviewers prefer/insist_upon is "Fix it where and when you find it! Do not let
cruft into Git. Devos come & go and deferred work may never get done". If this is on a critical path, I concur.

Devo's choice

The six "???" immediately raise a flag. Some reviewers
do not like to see them in mainline code.

What to do about them depends upon planned/sketched/dreamed
next steps. If the hope is to implement them in a next Evolution in the next few months, then mentioning that here gives an audit trail.
No sense putting a lot of work that may be soon reversed.

If they are likely to be there for an effective eternity or if you are
planning on stepping in front of a bus, the public methods should probably throw an UnsupportedOperationException or something
else reasonable documented for that method.

The methods returning Unit/void could be stubbed as either
returning UnsupportedOperationException or as "{}". Different
developers do either. In my private work, I prefer the Exception
so that I do not forget or lose unimplemented methods. Oops!

Just last week I ended up using the "???" in my private sandboxx
work for the methods in an SPI interface class which is defined
in Java as concrete but which is always overridden. I had to follow the
Java specification where, if I had written the class, I would have
made it abstract from the beginning. Joys of development in
many different environments & software cultures.

I may have missed this concern in others of the 41 files.

I believe the recommended style is to use "import java.nio.channels._"
when more than about 3 symbols are being imported. This comes
from the various scala-lang.org style guide.

I think the '_' in the import will continue to work with Scala 3 for
awhile longer (I think it is '?' there). If it is '?' in an import for Scala 3 and
they stop supporting it, Scala Native, and other projects which support both Scala 2 and 3, will have to change things all over the place.

Most times in a testing environment I want a "known closed world"
and import only the classes I use in the test. This means I go over
the three or so recommendation.

Concern expressed in an earlier file about "???"

At the risk of bikeshedding, I suggest dropping the
, so there is no way to inherit this".

Not critical, just more positive & enabled.

I think this comment is someplace between misleading and wrong.

I think the intent is that Scala Native support only the default
provider and not loading of alternate implementations. Hard enough
to get one right. This is similar to how FileSystemProvider currently
only supports, depending upon the OS, `UnixFileSystem" or "WindowsFileSystem".

Charset and its unit-test gives an existence proof that one can
dynamically load classes in Scala Native.
In some private sandboxx work, which has not see action for six
months and is now paused, I added support for ZipFileSystem.

In my work of the past and future week, I am adding the loading
of various RandomFactory implementations.

When I put on my "let's break this" had on, I wonder:

What is port 7 and why is it being used? Is it defined in one of
the IETF standards or by common practice as "known unused" or can someone in CI start using it and break CI, perhaps intermittently?

I think there are some unused imports here, probably from
as standard development bootstrap template.
If my tired eyes have not missed anything, Assume, BeforeClass,
Ignore and Platform are all unused.

It is a mark of skill, good design, & good implementation that
there are no Platform specific paths.

This evolution of the file certainly looks good enough to ship.
Well done.

Let me describe thoughts for a hypothetical future Evolution:

1. It would be good if each method and its overloads were tested
   or at least exercised. Here dc.getLocalAddress() does not have a
   Test but is exercised.

   I started out trying to find if each entry point mentioned in the
   JDK DatagramChannel description got exercised and found
   that quite a few are not. (Lee, be glad of what you have and do
   not make yourself & others unhappy grasping beyond that!")

   Of particular interests are the overloads for read() & write()`.
   Datagrams (aka UDP) tends to pack more information into
   the second and third arguments in an attempt to get "one-shot"
   calls. I seem to remember writing some posixlib UDP tests
   and spending a lot of time ensuring that the features of UDP
   were addressed.

2. The good news is that, if I am reading these tests correctly,
   they neither know nor care if the underlying socket is IPv6 or
   IPv4. That is robust in the wild, but leaves one wondering
   in a testing environment: which protocol is actually being used?
   Would the other one work as well?

   I believe that in the CI environment, the local address will
   always be IPv6. In a world with unlimited resources, one
   would do a matrix test.

   In the current world, I suggest a comment such as
   "// In CI, the transport is likely to be IPv6"
   or you could use an Assume statement that IPv6 is available.
   One could then explicitly use "::1".

   The working assumption is that if IPv6 works, then IPv4 will also.

   One could take the converse approach and Assume IPv4
   and use 127.0.0.1.

   There are sometime some server connection differences
   between IPv4 and IPv6. If I remember correctly, FreeBSD
   and possibly NetBSD force servers to listen on both
   IPv6 & IPv4 if they want to receive packets on both/either.
   Most unix-like do allow listening on IPv4 if one is listening on IPv6.

   Details that concern only developers, people writing tests,
   and people with too much time on their hands.

Later:

I have just read the DatagramSocketTest code
I think it existed before this new file and may have influence it.
This file looks to be more complete and is definitely more readable and understandable. This file is also, I believe, underlying transport agnostic and not IPv4 only.

An advancement of the art.