Multithreaded Java server: allowing one thread to access another one

Question

Hopefully the code itself explains the issue here:

class Server {

    public void main() {
     // ...
     ServerSocket serverSocket = new ServerSocket(PORT);
     while (true) {
      Socket socket = serverSocket.accept();
      Thread thread = new Thread(new Session(socket));
      thread.start();
     }
     // ..
    }

    public static synchronized Session findByUser(String user) {
        for (int i = 0; i < sessions.size(); i++) {
            Session session = sessions.get(i);
            if (session.getUserID().equals(user)) {
                return session;
            }
        }
        return null;
    }

}

class Session {
    public Session(Socket socket) {
     attach(socket);
    }

    public void attach(Socket socket) {
        // get socket's input and output streams
        // start another thread to handle messaging (if not already started)
    }

    public void run() {
     // ...
     // user logs in and if he's got another session opened, attach to it
     Session session = Server.findByUser(userId);
     if (session != null) {
      // close input and output streams
      // ...
      session.attach(socket);
      return;
     }

     // ..

    }
}

My question here is: Is it safe to publish session reference in Server.findByUser method, doesn't it violate OOP style, etc? Or should I reference sessions through some immutable id and encapsulate the whole thing? Anything else you would change here?

String sessionId = Server.findByUser(userId);
if (sessionId != null && sessionId.length() > 0) {
    // close input and output streams
    // ...
    Server.attach(sessionId, socket);
    return;
}

Thomas:

Thanks for your answer.

I agree, in a real world, it would be a good idea to use dependency injection when creating a new instance of Session, but then probably also with an interface, right (code below)? Even though I probably should have unit tests for that, let's consider I don't. Then I need exactly one instance of Server. Would it then be a huge OO crime to use static methods instead of a singletone?

interface Server {
    Session findByUser(String user);
}

class ServerImpl implements Server {
    public Session findByUser(String user) { }
}

class Session {
   public Session(Server server, Socket socket) { }
}

Good point on the attach(...) method - I've never even considered subclassing Session class, that's probably why I haven't thought how risy it might be to call public method in the constructor. But then I actually need some public method to attach session to a different socket, so maybe a pair of methods?

class Session {
    public Session(Socket socket) {
       attach_socket(socket);
    }

    public void attach(Socket socket) {
        attach_socket(socket);
    }

    private void attach_socket(Socket socket) {
        // ...
    }
}

It's true that allowing clients of Session to call attach(...) doesn't seem right. That's probably one of those serious mehods only the Server should have access to. How do I do it without C++'s friendship relationship though? Somehow inner classes came to my mind, but I haven't given it much thought, so it maybe a completely wrong path.

Everytime I receive a new connection I spawn a new thread (and create a new Session instance associated with it) to handle transmission. That way while the user sends in a login command, Server is ready to accept new connections. Once the user's identity is verified, I check if by any chance he's not already logged in (has another ongoing session). If he is then I detach the onging session from it's socket, close that socket, attach the ongoing session to current socket and close current session. Hope this is more clear explanation of what actually happens? Maybe the use of a word session is a bit misfortunate here. What I really have is 4 different objects created for each connection (and 3 threads): socket handler, message sender, message receiver and a session (if it's a good solution that's a different question...). I just tried simplyfing the source code to focus on the question.

I totally agree it makes no sense to iterate over session list when you can use a map. But I'm afraid that's probably one of the smaller issues (believe me) the code I'm working on suffers from. I should've mentioned it's actually some legacy system that, no surprise, quite recently has been discoved to have some concurrency and performance issues. My task is to fix it... Not an easy task when you pretty much got only theoretical knowledge on multithreading or when you merely used it to display a progress bar.

If after this, rather lengthy, clarification you have some more insight on the architecture, I'd be more than willing to listen.

Answer 1

You should start by making the Server class OO (i.e. not static) and use dependency injection in the Session class:

class Server {
    public Session findByUser(String user) { }
}

class Session{
   public Session(Server server, Socket socket){}
}

public void attach(..) has to be private to ensure encapsulation and proper initialization. A subclass could break the Session class otherwise like this:

class BadSession extends Session{

@Override public void attach(Socket socket) {
    //this is not initialized at this point

    //now the instance is broken        
  }
}

Calling attach from a client seems to be invalid, too.

The responsibility to attach the Socket to the Session should be part of the Server. This is the right place to decide which Session gets which Socket. As far as I do understand your code you are creating a Session with a Socket. Somehow you find out that the user already has a Session (with another Socket). Now you attach the current Session to this Socket. There is now the old Socket with two Sessions and the new Socket without a Session. I think the a traditional Session should have multiple Sockets not the other wayaround:

Session session = findSession(userId);
session.attach(socket);

class Session{
   List<Socket> sockets;
}

After this change the threads would not be assigned to Sessions but socket handlers, that process the input stream for one socket and change the Session accordingly.

Using synchronized for the method public static synchronized Session findByUser(String user) is not sufficient to ensure thread-safeness. You have to make sure that the look up of a session (by user) and the registration a session (if the user is not known) have to be atomic. The semantic should be analogous to putIfAbsent of ConcurrentMap. (Iterating over the session List is not efficient anyway. You should use a Map<Id, Session>.)

I hope this helps.