Skip to content

MultiServer.stop() lowers a flag but never closes the listening socket #21

Description

@dmccoystephenson

Problem

MultiServer has no working shutdown path. Three separate gaps combine into that.

The listening socket is never closed. serverSocket is assigned once
(src/SimpleServer/server/MultiServer.java:42) and read once
(:52). A search of the whole tree for .close() finds exactly one call,
socket.close() in MSThread.disconnect at src/SimpleServer/server/MSThread.java:44, which
closes a per-connection socket and not the listening one. Nothing releases the port.

stop() cannot interrupt an accept. It only lowers a flag:

// src/SimpleServer/server/MultiServer.java:28
public void stop() {
    listening = false;
}

The loop in start() re-tests that flag only after createNewServerThread() returns, and that
method blocks in serverSocket.accept() until a client arrives. A call to stop() therefore has
no effect until one more connection is accepted, which is the opposite of what a caller asking a
server to stop would expect. Closing serverSocket is the conventional way to break out of a
blocked accept.

listening is shared without synchronisation. It is a plain boolean
(src/SimpleServer/server/MultiServer.java:12), written by stop() and read by start() and
isListening(). A grep of src/ for volatile and for synchronized returns no matches
anywhere in the repository. Since stop() exists to be called by some thread other than the one
running the loop, the write is not guaranteed to be seen by the reader.

stop() is dead code today. The same grep shows no call site for it anywhere in the tree, so
none of the above is currently reachable. What is reported here is that the shutdown API the class
already exposes does not do what its name says.

Evidence

Read out of src/SimpleServer/server/MultiServer.java and confirmed by grepping src/ for
serverSocket, .close(), .stop(), volatile and synchronized, at d34c1da. No compiler
was available in the session in which this was filed, so the blocking behaviour of accept() is
taken from the java.net.ServerSocket contract rather than from an observed run, and is
UNVERIFIED in that sense.

Relationship to other issues

This is adjacent to #13, which covers the swallowed bind failure and the busy loop that follows
it, and to #12, which covers the unreachable per-connection disconnect. All three concern
lifecycle handling, and a single cycle could reasonably take them together. They are filed apart
because each has a distinct cause and a distinct fix.

Suggested fix

stop() could lower the flag and then close serverSocket, with the resulting exception in
createNewServerThread distinguished from a genuine accept failure so that an intentional
shutdown is not reported as an error. listening could be marked volatile.


This issue was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

drafted by Claude on behalf of Daniel Stephenson

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions