Verbesserungsvorschläge?

[NicoHatProbleme2]

Da ich letztens erst mit den Sockets angefangen habe, wollte ich fragen, was ich bei meinem System so verbessern könnte.

import java.net.*;
import java.util.ArrayList;
import java.util.List;
import java.io.*;
public class Client {
    List<ClientSideConnection> connections;
    public Client() {
        connections = new ArrayList<>();
    }
    public Client(int port, String id, String ip) {
        connections = new ArrayList<>();
        addConnection(port, id, ip);
    }
    public void addConnection(int port, String id, String ip) {
        connections.add(new ClientSideConnection(port, id, ip));
    }
    public String answerRequest(String request, ClientSideConnection connection) {
        if(request.equals("id")) {
            return "" + connection.id;
        }
        return null;
    }
    public void followInstruction(String request, ClientSideConnection connection) {
        
    }
    private class ClientSideConnection implements Runnable {
        String id;
        Socket socket;
        DataInputStream input;
        DataOutputStream output;
        Thread thread;
        String lastAnswer;
        public ClientSideConnection(int port, String id, String ip) {
            System.out.println("Creating Client");
            try {
                socket = new Socket(ip, port);
                input = new DataInputStream(socket.getInputStream());
                output = new DataOutputStream(socket.getOutputStream());
                System.out.println("Client created successfully");
                System.out.println();
            } catch (IOException e) {
                System.out.println("Failed to create connection to Server " + ip + " with port " + port);
            }
            this.id = id;
            thread = new Thread(this);
            thread.start();
        }
        @Override
        public void run() {
            while(socket.isConnected()) {
                try {
                    String serverStr = input.readUTF();
                    if(serverStr != null && !serverStr.equals("")) {
                        //request
                        if(serverStr.charAt(0) == 'R' && serverStr.charAt(1) == ':') {   
                            //"R:"
                            serverStr = serverStr.substring(2);
                            answerServerRequest(serverStr);
                        }
                        //instruction
                        if(serverStr.charAt(0) == 'I' && serverStr.charAt(1) == ':') {   
                            //"I:"
                            serverStr = serverStr.substring(2);
                            followServerInstruction(serverStr);
                        }
                        //answer
                        if(serverStr.charAt(0) == 'A' && serverStr.charAt(1) == ':') {   
                            //"A:"
                            lastAnswer = serverStr.substring(2);
                        }
                    }
                } catch (IOException e) {
                    System.out.println("Read Server String Error");
                }
                try {
                    Thread.sleep(1);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            connections.remove(this);
        }
        public void followServerInstruction(String request) {
            followInstruction(request, this);
        }
        public void answerServerRequest(String request) {
            try {
                output.writeUTF("A:" + answerRequest(request, this));
                output.flush();
            } catch(IOException e) {
                System.out.println("IOException in answerServerRequest");
            }
        }
        public String askFor(String s) {
            try {
                output.writeUTF("R:" + s);
                output.flush();
                while(lastAnswer == null) {}
                String answer = lastAnswer;
                lastAnswer = null;
                return answer;
            } catch (IOException e) {
                System.out.println("IOException at askFor() ClientSideConnection");
            }
            return null;
        }
        public void askToDo(String s) {
            try {
                output.writeUTF("I:" + s);
                output.flush();
            } catch (IOException e) {
                System.out.println("IOException at askToDo() ClientSideConnection");
            }
        }
        public void close() throws IOException {
            socket.close();
        }
    }

}

import java.io.*;
import java.net.*;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

public class Server implements Runnable {
    ServerSocket ss;
    HashMap<String, ServerSideConnection> connections;
    int maxConnections;
    boolean running;
    Thread thread;
    public Server(int port) {
        System.out.println("Constructing Server");
        connections = new HashMap<>();
        maxConnections = 100;
        try {
            ss = new ServerSocket(port);
            System.out.println("Server constructed successfully");
            System.out.println();
        }
        catch(IOException e) {
            System.out.println("Failed to create Server");
        }
        thread = new Thread(this);
        running = false;
    }
    public void setOpen(boolean shouldRun) {
        if(!this.running && shouldRun) {
            thread.start();
        }
        this.running = shouldRun;
    }
    @Override
    public void run() {
        System.out.println("Open search");
        while(running) {
            if(connections.size() < maxConnections) {
                try {
                    Socket s = ss.accept();
                    ServerSideConnection newConnection = new ServerSideConnection(s);
                    System.out.println("#" + connections.size() + " Client connected");
                } catch (IOException e) {
                }
            }
            try {
                Thread.sleep(1);
            } catch (InterruptedException e) {
            }
        }
        System.out.println("Closed search");
    }
    public void acceptConnections(int newConnections) {
        System.out.println("Waiting for " + newConnections + " connections");
        int accepted = 0;
        try {
            while(accepted < newConnections) {
                Socket s = ss.accept();
                ServerSideConnection newConnection = new ServerSideConnection(s);
                accepted++;
                System.out.println(accepted + "/" + newConnections + " accepted");
                System.out.println(connections.size() + " Clients on the Server");
            }
        }
        catch (IOException e) {
            System.out.println("Exception");
        }
    }
    public void connectionCreated(ServerSideConnection connection) {
        connection.id = connection.askFor("id");
        connections.put(connection.id, connection);
    }
    public void followInstruction(String request, ServerSideConnection connection) {
    }
    public String answerRequest(String request, ServerSideConnection connection) {
        return null;
    }
    protected class ServerSideConnection implements Runnable {
        String id;
        Socket socket;
        DataInputStream input;
        DataOutputStream output;
        Thread thread;
        String lastAnswer;
        public ServerSideConnection(Socket s) {
            socket = s;
            try {
                input = new DataInputStream(s.getInputStream());
                output = new DataOutputStream(s.getOutputStream());
                thread = new Thread(this);
                thread.start();
            } catch (IOException e) {
                System.out.println("Error with creating streams");
            }
            connectionCreated(this);
            System.out.println("Connection id: " + id);
            lastAnswer = null;
        }
        @Override
        public void run() {
            while(socket.isConnected()) {
                try {
                    String clientStr = input.readUTF();
                    if(clientStr != null && !clientStr.equals("")) {
                        //request
                        if(clientStr.charAt(0) == 'R' && clientStr.charAt(1) == ':') {   
                            //"R:"
                            clientStr = clientStr.substring(2);
                            answerClientRequest(clientStr);
                        }
                        //instruction
                        if(clientStr.charAt(0) == 'I' && clientStr.charAt(1) == ':') {   
                            //"I:"
                            clientStr = clientStr.substring(2);
                            followClientInstruction(clientStr);
                        }
                        //answer
                        if(clientStr.charAt(0) == 'A' && clientStr.charAt(1) == ':') {   
                            //"A:"
                            lastAnswer = clientStr.substring(2);
                        }
                    }
                } catch (IOException e) {
                    System.out.println("Read Server String Error");
                }
                try {
                    Thread.sleep(1);
                } catch (InterruptedException e) {
                }
            }
            connections.remove(id);
        }
        public void followClientInstruction(String request) {
            followInstruction(request, this);
        }
        public void answerClientRequest(String request) {
            try {
                output.writeUTF("A:" + answerRequest(request, this));
                output.flush();
            } catch(IOException e) {
                System.out.println("IOException in answerClientRequest");
            }
        }
        public String askFor(String s){
            try {
                output.writeUTF("R:" + s);
                output.flush();
                while(lastAnswer == null) {}
                String answer = lastAnswer;
                lastAnswer = null;
                return answer;
            } catch (IOException e) {
                System.out.println("IOException at askFor() ServerSideConnection");
            }
            return null;
        }
        public void askToDo(String s) {
            try {
                output.writeUTF("I:" + s);
                output.flush();
            } catch (IOException e) {
                System.out.println("IOException at askFor() ServerSideConnection");
            }
        }
        public void close() throws IOException {
            socket.close();
        }
        
    }

}

 

[KonradN]

Also beim Client sind mehrere Verbindungen zum Server eher selten zu finden. Da ist die Kernidee ja, dass man da mehrere Clients hat, die sich zu einem Server verbinden.

Beim Code bin ich eher ein Freund von Lambda Expressions oder Methodenreferenzen. Eine Klasse erbt also nicht von Runable und hat dann eine Methode run (Das sagt ja nichts aus!) sondern es gibt eine Methode, die eben keinen Parameter hat und die vom Namen her sagt, was sie macht und die wird dann verwendet z.b. als this::leseInSchleifeVomSocket oder so.

In der Vergangenheit wäre noch ein Hinweis gekommen bezüglich Async Aufbau der Connection - Das erspare ich mir mit Hintergrund virtuelle Threads von Java 19 (noch in Preview) -> Das arbeiten mit (virtuellen) Thrads wird hier also definitiv der Java Weg (in der nicht zu fernen Zukunft) sein. Aber bis das kommt bitte im Hinterkopf behalten: Dieser Ansatz ist untauglich für viele Clients. So ein Server wird nur eine begrenzte Anzahl Clients verwalten können!

Security Hinweis: Das ist natürlich ein einfacher Socket - der ist unsicher / nicht verschlüsselt und hat damit massive Probleme. Also nicht nur, wenn man darüber etwas macht mit Anmeldung und so sondern auch, wenn man da z.B. personenbezogene Daten versenden würde (die müssen immer verschlüsselt sein ... Es gab Abmahnungen wegen http Webseiten und das setze ich hier einmal gleich)
Daher ist das ein netter Ansatz zum lernen, aber nichts für die Produktion. Da kann man aber etwas ähnliches aufbauen z.B. über Websockets und so. Also prinzipiell ähnlich, nur der Socket läuft dann über einen Schnittstelle, die auch direkt Verschlüsselung unterstützt und so.

Das wären so ein paar Rückmeldungen von meiner Seite auf die Schnelle. Hoffe, das war so in etwa, was Du an Hinweisen erwartet hast.

 

[LimDul]

Was mir sonst auffällt.

Positiv: Exceptions werden nicht sang und klanglos geschluckt
Negativ: Die eigentliche Exception Meldung (e.getMessage()) wird weder ausgegeben / protokolliert. IOExceptions gibt es viele mit verschiedenen Gründen. So kannst du nicht entscheiden, war ein Timeout, ein Connection Refused etc. Deswegen würde ich dir empfehlen mindestens die Message mit auszugeben, das hilft im Fehlerfall.

Aber es ist schon mal deutlich besser bzgl. Exception Handling was man sonst oft zu Gesicht bekommt.

 

[NicoHatProbleme2]

Also beim Client sind mehrere Verbindungen zum Server eher selten zu finden. Da ist die Kernidee ja, dass man da mehrere Clients hat, die sich zu einem Server verbinden.

Beim Code bin ich eher ein Freund von Lambda Expressions oder Methodenreferenzen. Eine Klasse erbt also nicht von Runable und hat dann eine Methode run (Das sagt ja nichts aus!) sondern es gibt eine Methode, die eben keinen Parameter hat und die vom Namen her sagt, was sie macht und die wird dann verwendet z.b. als this::leseInSchleifeVomSocket oder so.

In der Vergangenheit wäre noch ein Hinweis gekommen bezüglich Async Aufbau der Connection - Das erspare ich mir mit Hintergrund virtuelle Threads von Java 19 (noch in Preview) -> Das arbeiten mit (virtuellen) Thrads wird hier also definitiv der Java Weg (in der nicht zu fernen Zukunft) sein. Aber bis das kommt bitte im Hinterkopf behalten: Dieser Ansatz ist untauglich für viele Clients. So ein Server wird nur eine begrenzte Anzahl Clients verwalten können!

Security Hinweis: Das ist natürlich ein einfacher Socket - der ist unsicher / nicht verschlüsselt und hat damit massive Probleme. Also nicht nur, wenn man darüber etwas macht mit Anmeldung und so sondern auch, wenn man da z.B. personenbezogene Daten versenden würde (die müssen immer verschlüsselt sein ... Es gab Abmahnungen wegen http Webseiten und das setze ich hier einmal gleich)
Daher ist das ein netter Ansatz zum lernen, aber nichts für die Produktion. Da kann man aber etwas ähnliches aufbauen z.B. über Websockets und so. Also prinzipiell ähnlich, nur der Socket läuft dann über einen Schnittstelle, die auch direkt Verschlüsselung unterstützt und so.

Das wären so ein paar Rückmeldungen von meiner Seite auf die Schnelle. Hoffe, das war so in etwa, was Du an Hinweisen erwartet hast.

Zum Security Hinweis: Was könnte man da denn anderes nutzen? Wenn ich das einfach selber verschlüssele, geht das dann?

 

[KonradN]

Was könnte man da denn anderes nutzen?

Websocket wäre da das Stichwort, dass ich mir ansehen würde. Man kann das aber auch einfach ssl Verschlüsseln - das bieten diverse Libraries wie z.B. netty.

Wenn ich das einfach selber verschlüssele, geht das dann?

Ja, das geht auch. Dabei ist aber aufzupassen, was du wie machst / überträgst. Da gibt es halt auch diverse Problematiken, die man beachten sollten. Daher ist es nach meiner Erfahrung mit am Besten, wenn man da auf fertige Implementationen zurück greift. In der Regel ist man ja kein Security Experte und die ganzen Schwachstellen, die immer wieder überall gefunden werden, zeigen ja, dass es in dem Bereich nicht unproblematisch ist.

Daher greife ich bei sowas gerne auf verbreitete Frameworks zurück und nutze da die Best Practices - da ist die Chance mit am höchsten dass:
a) da schon der eine oder andere mit mehr Ahnung über den Code geschaut hat
b) bei Problemen diese auch bekannt werden

Denn noch schlimmer als eine Schwachstelle im Code zu haben, ist es, eine bekannte Schwachstelle im Code zu haben für die es Exploits gibt und man hat einfach keine Ahnung