im making a networked game that has a server which creates a clientHandler thread every time a client joins. I want to ask the first client that joined if it wants to start the game every time a new client joins, giving it the current number of players connected. Writting through the clientHandlers printwritter gives a nullPointerException, even though ive started the thread before doing this. what could be the problem?
Here is the server code:
`public class Server implements Runnable{
private ArrayList<ClientHandler> handlers = new ArrayList<>();
private ArrayList<Player> players = new ArrayList<>();
private Game game;
private boolean start;
public static void main(String[] args) {
Server server = new Server();
Thread s = new Thread(server);
s.start();
}
public void login(String name){
//todo
for (ClientHandler c : handlers){
if (c.getName().equals(name)){
alreadyTaken(name);//todo
}
else{
players.add(new HumanPlayer(name,c));//todo
}
}
}
public void setStart(){
start = true;
}
private void alreadyTaken(String name) {
}
public void setTurn(ServerHandler sh){
//todo
}
public void updateView(){
}
public String hello() {
return "Hello"; //?
}
public void place(String s){
}
@Override
public void run() {
ServerSocket serverSocket;
try {
serverSocket = new ServerSocket(1800);
} catch (IOException e) {
throw new RuntimeException(e);
}
System.out.println("----Server----");
while (!serverSocket.isClosed()) {
try {
Socket socket = serverSocket.accept();
ClientHandler handler = new ClientHandler(socket,handlers,this);
handlers.add(handler);
Thread h = new Thread(handler);
h.start();
System.out.println("A new client has connected");
System.out.println(handlers.get(0));
handlers.get(0).out.println("START? "+ handlers.size());
if (start){
System.out.println("start request works");
}
} catch (IOException e) {
throw new RuntimeException(e);
}
}
}
}
`
And here's the client handler code:
`public class ClientHandler implements Runnable{
private Socket socket;
private ArrayList<ClientHandler> handlers;
private Server server;
public PrintWriter out;
private BufferedReader in;
private String name;
public ClientHandler(Socket socket, ArrayList<ClientHandler> handlers, Server server){
this.socket = socket;
this.handlers = handlers;
this.server = server;
}
public void broadcastMessage(String msg){
System.out.println("Broadcasting");
for (ClientHandler s : this.handlers){
s.out.println("Player: " + msg);
}
}
public static String removePrefix(String s, String prefix)
{
if (s != null && s.startsWith(prefix)) {
return s.split(prefix, 2)[1];
}
return s;
}
public String getName(){
return name;
}
@Override
public void run() {
try {
out = new PrintWriter(new OutputStreamWriter(socket.getOutputStream()),true);
in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
new Thread(() -> {
while(socket.isConnected()){
String msg;
try {
msg = in.readLine();
while(msg!=null){
switch (msg.split(" ")[0]){
case "LOGIN":
name = removePrefix(msg,"LOGIN ");
server.login(name);//todo
break;
case "HELLO":
server.hello();//todo
break;
case "PLACE":
server.place(removePrefix(msg,"PLACE "));
break;
case "QUIT":
//todo
break;
case "STOP":
//todo
break;
case "START":
server.setStart();
default:
broadcastMessage(msg);
break;
}
msg = in.readLine();
}
} catch (IOException e) {
throw new RuntimeException(e);
}
}
}).start();
} catch (IOException e) {
throw new RuntimeException(e);
}
}
}`
I tried making a method in the client handler class which does the same thing. The server would just call that instead of writting directing through the PrintWriter, but i got the same error.
Starting a thread does not mean it is guaranteed to actually finish executing the first statement in its run() method before start() returns. In fact,
start() returns as soon as it can.start() returns is 'correct' - that is fine. A JVM that doesn't is also fine. Generally you don't want threads, because nothing is predictable anymore. At the very least you want to keep 'inter-thread comms' down to a minimum. Anytime a single field is used from more than one thread, things get very tricky.What you need is synchronized or other tools to insert predictability in this code.
Your ClientHandler's run() code starts another thread for no reason. Take all that out, your run() method in ClientHandler should set up out and in and then immediately do while (socket.isConnected())
At the very basic level, make a locker object and use notify/wait:
private final Object lock = new Object();
@Override public void run() {
try {
synchronized (lock) {
out = ...;
in = ...;
lock.notifyAll();
}
while (socket.isConnected()) { ... }
out definitely cannot be public here, you can't refer to a stream from multiple threads and expect things to work out!
Just 'fixing' your code involves then using something like:
public OutputStream getOutputStream() {
synchronized (lock) {
while (out == null) {
lock.wait();
}
}
return out;
}
Which will ensure that any thread that wants the out will wait for the other thread to get far enough, but, really, this is just setting you up for another 20 threading problems down the line. Instead, you want one object responsibile for all communication (both outgoing and incoming), and a concurrency-capable queue (there are various collections in the java.util.concurrent package good for this). Then:
.wait() or nicer API from e.g. java.util.concurrent, or, use callback hell - you pass a closure with the code to run once the result is received.