javalib `SocketServer.close()` does not unblock thread in `accept()` on Linux

[tassiluca]

Description

When running on Linux, calling close() on a SocketServer does not correctly unblock the thread blocked on accept() throwing a SocketException as expected. This results in the server thread remaining blocked indefinitely.

Expected Behavior

When server.close() is called, any threads blocked in accept() should be interrupted with a SocketException.

Actual Behavior

The accept() call does not return raising an exception after server.close() is called. The thread running the accept remains blocked.

Steps to Reproduce

You can reproduce the issue by running the failing test provided in this repository.
This issue is also consistently reproduced in CI. You can see a failing test run on Ubuntu here.

Note: the buggy behaviour is not limited to Ubuntu, rather Linux-specific.

[LeeTibbert]

tasiluca,

Thank you for reporting this and, especially, for the reproducer.

Do you know, off the top of your head, if this happens on macOS or Windows?

Later:

1. I just checked the log files from the CI run you cited. From the names of the passing
   and failing tests, it appears this fails on Linux but there is evidence that it
   works on macOS and Windows.

2. Linux & macOS share most of their implementation. The Windows implementation differs.
   IIRC the Linux & macOS paths differ if there is a timeout involved. When I chase this,
   I'll have to check the reproducer repository code to see if there is a timeout. Otherwise,
   it might be down to os differences in socket.close().

I am one of the SN contributors who has done a lot of network implementation.

I am somewhat surprised by the situation you describe. I'd expect that
when the socket got closed, any other socket using it would fail. Usually
the os interrupts pending/waiting I/O using the socket when it gets closed.

(Perhaps the ServerSocket.close() never gets all the way down to the
os and is swallowed somewhere in a chain of Impl calls?)

I am on holiday until early September, so any fix or discussion will be delayed.
I hope you can workaround this condition until then.

I think my next step would be to see what happens with SN program
using posixlib calls.

I did take the time to do initial triage.

• I read the base topic a second and third time carefully after my initial scan.

• I refreshed my memory of the existing SN java.net code, the ServerSocket and
  various *PlainSocket* classes. I did not write the sections of interest but I saw
  nothing unusual (which is why we write & appreciate unit-tests). I usually avoid
  scripted-tests but this scenario might be a candidate, if I am not clever enough
  to be able to shoe-horn it into a unit-test.

• I read, but did not execute, the code in the repository you kindly supplied.
  I did not see anything obvious, to me, off or unusual. Accessing a socket
  from another process was un-usual (outside the realm of discourse) when
  sockets were designed but are (mildly & unwisely) acceptable, to some, practice today.

So, plenty to keep me busy when I get back.

[LeeTibbert]

@tassiluca

I've been thinking about how to create a Scala Native unit-test.

What is the license for the reproduction-socket-bug
repository. I went looking for one and could not find such.

SN devos must be very careful to respect license conditions. Apache, MIT, and, easiest for devo of all Creative Commons
or public domain. All work. I think various BSD licenses also work but always have to check my memory there.
Anything GNU is not available.

The ideas of the repro are, I believe, OK for me to read. I'd like to re-work the actual code and need/want to
make sure I respect the license. If the license is restricted, I can write something from a clean sheet of paper/electrons.

Please advise & thank you. for any information.

Lee

[LeeTibbert]

Notes to any future devo (and to give myself a hot start when I return from holiday):

1. Once a unit-test, or at least reproducer is in place, I believe a promising concept for
   a fix is to add a socket.shutdown() call on the underlying OS fd some where in
   the close() path.

2. It is unclear/unproven to me if the change needs to apply to both (client) Socket.scala
   and ServerSocket.scala. It appears necessary on the latter on Linux. Does it have
   any deleterious effect, other than a longer code path, on macOS, Windows, or the BSDs?

3. AbstractPlainSocketImpl.scala has method shutdownInput and shutdownOutput.
   One could create a method shutdownBoth and modify ServerSocket#close to
   call impl.shutdownBoth() before it calls impl.close().

4. Alternately, one could modify AbstractPlainSocketImpl#close() to call socket.shutdown(SHUT_RDWR)
   (insert any required Windows fix here) before it calls unistd.close().

Probably which of the alternative depends on how many cells in the config matrix need a fix.
If many, the #4 fix is probably better even though it requires more testing.

[tassiluca]

Hi @LeeTibbert, thank you very much for the quick response!

I’ve just added a public domain license (Unlicense) to the reproduction repository of the bug.

Although I’ve never contributed, I’m happy to try to help in my free time, with your guidance, if you think I can be useful while you’re on holiday. I can’t promise anything, but I’m willing to give it a shot.

[LeeTibbert]

tassiluca

The clear (and enabling) license helps.

It would be great if you could give this issue some time. I am 9.5 toes out the door for
holiday, so please excuse me for dropping & running.

I do not know your background, but this Issue might be a good exercise in practical Software Engineering.
Not just coding but considering things like comparative design, introduced code path lengths and
maintainability issues.

I, and I believe other SN contributor's, welcome any time you are able to give to improving Scala Native.

I'll write down some ideas here and you can see how far you get. We can co-ordinate when I get back,
or post here and someone may be able to answer.

Part 1

As mentioned, from the information in the base topic, I believe that there are two fundamental axis
to this Issue. The first is the "where does it appear; i.e. which OSen?" axis. You are way ahead
by having a reproducer for each of the three major supported operating systems.

The second axis is "Is the defect limited to ServerSocket or does it also appear in (client) Socket?"
This affects design. If it occurs in both, it makes to at least start out putting any fix in a common
code path (probably SN javalib/net AbstractPlainSocket#close). If it affects only ServerSocket
then it makes no sense to lengthen the client close path (executing more instructions and
a transition to kernel mode).

A good high level design makes the rest easier, and minimizes the chances of "Oh, you fixed the
left shoe but not the right". The question before the house becomes: Can we catch the
same behavior from the client? I believe that will take some experimentation and my
vacation is unlikely to block that effort.

Now getting a client socket to block long enough to be interrupted by an asynchronous close will
take some cleverness and luck. The closest client call analogous to the SocketServer#accept
is, from the Java 24 JDK docs (and probably similar all the way back to Java 8. I like to use
the most current docs for at least initial reference, sometimes having to fall back to the JDK 8 docs).

public void connect(SocketAddress endpoint,
 int timeout)
             throws IOException

The 'endpoint' would be a address known to be not-known to bind, Perhaps a 10.mumble
address on the local network or a 192.168.mumble address behind firewall. Something
which will not immediately fail but which will block for the timeout period unless
interrupted. As an aside, all of this is IPv4. IPv6 should have similar behavior.
In a better world, that would be a third axis of testing.

I'd probably specify the timeout for 10 or 20 seconds, not zero (Infinite). That gives time for
a second thread to asynchronously close, similar to your existing ServerSocket test.
Now, the asynch close is pretty brutal, and I would (almost) never recommend it in Real Life,
but it is handy here.

The JDK 24 gives the two major possible errors in the 'connect'ing process when interrupted.

The results of this test, evaluated on each of the three operating systems (and the loose and risky assumption
that FreeBSD, NetBSD, etc. work the same way. Get the core working, then check the periphery stretch goals.)

A 'connect' which times out should probably be tested with a, say, 30 second timeout. If it still times out
that is evidence for making the change in the common path (AbstractPlainSocketImpl.scala#close).
The question there is does it need to be behind a 'LinktimeInfo.isLinux` or do other platforms (operating systems)
need it also. If it appears that other platforms work both with and without it, how much do we believe the "without"
case? A design, risk, & maintainability decision. Is it worth increasing the code path on platforms where the
current "without" appears to be working. (my guess is that the answer is "YES" for ServerSocket and "no" for
client Socket but evidence rules.)

[LeeTibbert]

Part 2

A related but separate endeavor would be to clone the Scala Native repository, publish it locally (to you), and run
your existing test to see if it solves that problem. If you are able to get a test of the client to hang, is a fix need there?
If yes, does the proposed Server fix also fix that.

- Do the SN unit-tests successfully run locally (to you), including the scripted tests.
- Do the SN unit-tests run on SN CI - This probably means a 'Draft' PR.

• I suggest starting by reading the Scala Contributor License Agreement, a.k.a CLA at
  CLA. That way you know early on
  what is being asked of you and you can agree or not. Legalism is an unfriendly, but
  necessary entry point.

• The Scala Native doc (I am not sure if there is an Italian
  or other language version of that page) has a 'Contributor’s Guide'. That Guide is woefully out of date and incomplete
  but it has a "Contributing Guidelines" section which is up-to-date and essential to understand.

  It is not mentioned in that section, but Scala Native contributors can not never look at an
  Oracle or current JDK implementation, not even a bit or in passing. Looking at the published
  JDK API documentation is OK. Apache Harmony is/was an allowable JDK implementation but
  work on it ended 15 or so years ago. The intervening years have not been kind to it.
  I believe some liberally licensed Android code can be both studied and ported. The bottom
  of the SN LICENSE.md has a recent case of such.

• After all that, one can clone the SN repository & install the necessary software to build SN itself.
  You probably already have 'sbt' & 'scala'. IIRC, Ubuntu 25.04 does not have the most current
  released stable LLVM (llvm-20?) but it is recent enough that I have been able to use it.

  • If your scala version is >= 3.5 you can use it to format .scala sources. If it is earlier,
    I recommend downloading & installing the latest [scala-cli](https://github.com/VirtusLab/scala-cli
    and using "scala-cli fmt foo.scala". I find the SN build configuration to be incredibly rejecting
    of whitespace & indentation and tend to format my code before each Scala run. I think
    some other SN devos use and IDE which handles formatting better than my editor.

• I suggest exercising the 'testsJVM3/test/', 'tests3/test', 'testsJVM3/testOnly *.net.ServerSocketTest",
  and 'tests3/testOnly *.net.*Test' commands. The 'testsJVM3' should run the test using JVM and Scala 3.
  the 'tests3' use Scala Native javalib and Scala 3. Later. running 'scripted' tests will be
  wise because there are some network tests there (run-scripted; takes 1/2 hour or so).

  Eventually, for a PR, running 'test-tools' and various tests on Scala 2.12 & Scala 2.13 will become
  necessary.

  For now, starting easily and becoming familiar with the SN unit-test environment is a friendlier
  approach.

• Once you are have some level of ease and confidence in your local unit-tests, then you can
  do a 'publish-local' (or has it done a protean morph to 'publish-release'?). That should put
  some files 'org.scala-native*' under your '~/.ivy2/local/' directory'. You then change
  the SN dependency from mumble0.5.8 to mumble0.5.9-SNAPSHOT and run your
  existing ServerSocket test. Sometimes I put a/a_few 'printf' statements (with a // FIXME
  comment to make the easier to find later) in the code I am changing to convince
  my self that the changed code actually is being executed. Brute force debugging but
  effective. Perhaps others have a more elegant solution (for simple code one can use
  'gdb' but I find the trail soon becomes more complex).

• I'll try to check back this week to answer any questions you may have on this setup.

• I can also be on standby after I return in order to assist. In general, if you run
  into an issue, it is probably worth 15 minutes to figure out; after that it is probably
  worth asking because the chaotic boundary of the simple has been crossed into
  the incredibly complex. We/SN are/is trying to make the process of getting
  new devos up to speed less frustrating and full of wizardry.

• I think the French expression for wishing good fortune for a challenging but do-able task
  is "bon courage".

[LeeTibbert]

@tassiluca

Thought I would co-ordinate with you as your time allows.

I am now back from vacation and have recovered from several hardware hardware failures.
What needs to be done to move this Issue forward? I do not want to do anything than might
stomp on or duplicate any progress that you might have been able to make. Obviously, I did not make
much progress on this in the last month or so.

Thank you.

[tassiluca]

Hi @LeeTibbert! Sorry for not having answered sooner.
What I did so far:

• I verified, following your suggestions, and the issue is not limited to ServerSockets but also to client Sockets. Attempting to close a socket that is blocked on the connect does not unblock it. Moreover, it turned out this second test fails also on MacoOS, not only Linux.
• From the documentation of the Linux man page there is a paragraph on multithreading and close that explain our issue:

consider the following scenario where two threads are performing operations on the same file descriptor:

(1) One thread is blocked in an I/O system call on the file descriptor. For example, it is trying to write(2) to a pipe that is already full, or trying to read(2) from a stream socket which currently has no available data.

(2) Another thread closes the file descriptor.

The behavior in this situation varies across systems. On some systems, when the file descriptor is closed, the blocking system call returns immediately with an error.

On Linux (and possibly some other systems), the behavior is different: the blocking I/O system call holds a reference to the underlying open file description, and this reference keeps the description open until the I/O system call completes. (See open(2) for a discussion of open file descriptions.) Thus, the blocking system call in the first thread may successfully complete after the close() in the second thread.

• I created two tests in my scala-native fork that correctly fail
  • This is for the client socket, failing also on MacOS
  • This is for server socket
  • they can be improved :)
• Following your suggestions I added shutdownBoth in AbstractPlainSocketImpl#close method just before the close is performed. This allows the server socket test to pass, but it doesn’t always make the client test pass: sometimes it still fails. I suspect that this is due to the fact that, since the thread is blocked on connect (the socket is not already connected), the shutdown has no effect (this is also the reason why the shutdown is inside a try-catch: if an attempt is made to close an unconnected socket, it fails). I stopped here, I didn't have yet the time to investigate more deeply
  • If it helps I can open a draft PR :)

[LeeTibbert]

@tassiluca

You have made an astounding amount of progress. I hope the effort has been rewarding, at some level.

Let me respond in a series of separate replies, each focused on one issue. Wish we had a whiteboard!

Let me know if the discussion gets too tangled or if I miss something.

[LeeTibbert]

I verified, following your suggestions, and the issue is not limited to ServerSockets but also to client Sockets. Attempting to close a > socket that is blocked on the connect does not unblock it. Moreover, it turned out this second test fails also on MacoOS, not only
Linux.

This is a key finding. It also helps locate an efficient fix to the method you discovered: javalib/src/main/scala/java/net/ AbstractAbstractPlainSocketImpl.scala method close().

That method has an existing block:

      if (isWindows) WinSocketApi.closeSocket(fd.handle)
      else unistd.close(fd.fd)

I propose/suggest adding a block immediately before that (conceptual, not compiles):

    if (!isWindows) {
      // See Issue #4420 for full discussion.

      // FIXME
    }

Now the code at FIXME is 1 line of code but 100 lines of comments, starting with something like:

     The OS fd at this point may be quiescent or a thread may be using it whist it blocks in
     "accept()" or "connect()". Shutting down the OS fd will (hopefully) interrupt any pending "accept()"
     call and will (hopefully) cause any pending "connect()" call to return with the connection proceeding
     asynchronously (only to report  closed connection the next time that thread uses the OS fd).

     // DON'T DO THIS AT HOME!
     Because we do not know the current state/usage of the OS fd, we can distinguish when
     the "shutdown()" call should return success or error. Hence, ignore that return status.  

Feel free to rework and improve.

---

In my studies over the weekend I found that some of the SN ServerSocketTest bind tests failed if I did
not shutdown both read & write on the OS socket. This confirms what you report.

It is not evident from the javalib code, but the posixlib socket.shutdown() defines a (pseudo)
constant socket. socket.SHUT_RDWR which will/should shutdown, as best it can, both read and write on
the OS fd.

(A robustness 2+ after we get this Issue fixed would be to change AbstractPlainSocketImpl shutdownInput()
and shutdownOutput() to:

• use socket.SHUT_RD and socket.SHUT_WR rather than 0 & 1, as they now do. Not ever operating system
  may use the 0 & 1 values. I'm actually kind of surprised that macOS does not complain.

• use C strerror() to convert an error errno to a human readable text string. There are a (too large) number of
  examples of that in the SN codebase.
  )

[ekrich]

Hi Lee, at one time we, or me an another contributor, discussed a solution for mapping errors on the Windows side because as I recall providing the messages on the non-Windows left a hole for the Windows side. Let me know if you recall or remember the issue.

[LeeTibbert]

I created two tests in my scala-native fork that correctly fail.

Over the weekend, I translated your at-that-time ServerSocket test from
scalatest to the JUnit 4 format used by SN. I'll post that code at the bottom of this
reply. You/we do not have to use it as is, but it is a bootstrap.

I do not know how fluent you are in translating scalatest to JUnit 4. I have not
yet looked at the newer server & client tests in your repository. If you like,
I could take a run at translating them from scalatest to JUnit 4. The offer is
to remove a pain point, not to deprive you of a learning opportunity. Not
everything can happen at once.

Running SN tests on both JVM & Scala Native

Whilst working in javalib, I and others have found it quite useful to be able to do
A/B testing were a given JUnit test class is first run on JVM and then on Scala Native.
For a Scala 3 example:

sbt> testsJVM3/testOnly *.ServerSocketTest
sbt> tests3/testOnly *.ServerSocketTest

Imagine my surprise & consternation over the weekend when my sbt> testsJVM3/testOnly *.ServerSocketTest
did not run while my sandbox sbt> testsJVM3/testOnly *.Leet_ServerSocketTest did. See SN Issue #4449.

As you are preparing a draft PR and getting it to work on your systems, probably the first edit should be
to remove the ServerSocket line from unit-tests/jvm/src/test/resources/DenylistedTests.txt.

Than you ought to be able to do the JVM/SN tests above on unaltered ServerSocketTest. This will
establish a baseline.

You probably have your own development pathway. I find it useful to create an exact copy of an existing
file with a ".original" suffix before I start making changes. Thus I would copy 'ServerSocketTest.scala'
to ServerSocketTest.scala.original. I find this incredibly useful when, just before I commit & push a PR,
I compare my changed files to the originals to see if only my intended edits are going into the PR.

Newly created files get a ".scala.created.original" suffix, then copied to the normal ".scala" or such.
I find this helps me keep track of newly created files so that I remember to add them to git for the PR.
Saves CI failures due to "Missing file".

JUnit 4 ServerSocket

This may look familiar to you...

@Test def close(): Unit = {
    val s = new ServerSocket(0)
    s.close
    assertThrows(classOf[SocketException], s.accept)
    // socket already closed, all paths.                                        
  }

 // Issue #4220                                                                
  @Test def closeWithActiveAccept(): Unit = {
    val nAcceptors = 1
    val acceptStarted = new CountDownLatch(nAcceptors)
    val countdownWaitPerAcceptor = 5 // seconds                                 

    val serverSock = new ServerSocket(0)

    def acceptor(acceptSocket: ServerSocket, id: Integer): Future[Socket] =
      Future {
        acceptStarted.countDown()
        // "Those who wait also serve" - John Milton, "On His Blindness"        
        acceptSocket.accept()
      }

    val acceptors: List[Future[Socket]] =
      (1 to 1).toList.map(acceptor(serverSock, _))

    val totalCountdownWait = nAcceptors * countdownWaitPerAcceptor

    val started = acceptStarted.await(
      totalCountdownWait,
      TimeUnit.SECONDS
    ) // wait until accept is about to be called                                

    if (!started)
      fail(s"acceptors did not start after ${totalCountdownWait} seconds!")

    Thread.sleep(2_000) // Give each Future plenty of time to get to accept().  

    serverSock.close() // should cause every acceptor to unblock & complete     

 assertThrows(
      classOf[SocketException],
      Await.result(Future.sequence(acceptors), 20.seconds)
    )
  }

  @Test def soTimeout(): Unit = {
    val s = new ServerSocket(0)
    try {
      val prevValue = s.getSoTimeout
      s.setSoTimeout(prevValue + 100)
      assertEquals(prevValue + 100, s.getSoTimeout)
    } finally {
      s.close()
    }
  }