- nevermind, use 200_successes recommendation use 200_successes recommendation. It is good.
- nevermind, use 200_successes recommendation. It is good.
- nevermind, use 200_successes recommendation. It is good.
Update: The question: "My solution was to create a separate command processor thread, which would then span a new thread for each command. Any comments on my approach to ensure that commands are processed in the order that they are received?"
There are two parts to processing the commands... the first part is that they should be acknowledged to the client, and it is appropriate that the ack is in the correct order. There is no requirement that the commands are actually processed in that same order.
Notes:
- *I only just saw that the XML input file was in the question, but 'hidden'... I edited the Q and made it show up properly)
- I never reviewed this part of your problem the first time around.... it is 'bigger' than just the server-socket thread.
- the more I look in to it, the more problems I am seeing..... this is frustrating...
- your server, as it stands, does not notify the client when it receives each command, it just notifies when it receives the first data... and does not print each command....
The network server I described above is pretty typical. Here are some results from google which describe it:
- The Oracle tutorial
- another Oracle tutorial
- A Pattern/Framework for Client/Server Programming in Java
- A multi-threaded socket-based server <-- horrible code style!!!
The point is that they all follow the same pattern, and it's a good one.
Now, your problem is that a client may send more than one request. This is a problem because.... you need a protocol to support it.
- the protocol I imagined in a simple case was that the client 'dumps' the request, and the server processes it, and responds.
- if the client can send multiple requests... how do we know when one request ends, and the next one starts?
- XML input cannot just be read from the socket... because any additional client requests will cause the XML to become invalid....
- your current code just reads a file from disk, and dumps it to the socket.... and does not 'parse' the input in to individual requests. Actually, your code strips all the newlines from the input XML before sending it, so the XML is only one one line.... you should comment that, because it is not obvious.
(削除) One of the big 'gotcha's' with network processing is that data does not arrive when you expect it. For example, in your client, it may just hang forever.... because you never close the OutputStream in the client..... in fact.... **Your server WILL NOT complete the parsing of the client's data.... (削除ここまで) --- I just realized that you force all the XML on to one line, and that my previous comment is wrong... you need to document these things.
I had previously assumed that the protocol was a simple:
- client opens socket
- dumps request on socket's OutputStream
- flushes stream
- closes stream
- listens on socket's InputStream for server response.
- processes responses until InputStream is closed
- done.
But, as 200_success points out, it is nothing of the sort.... you should be forcing the data through the socket, and you are not! You are not flushing the output stream, and you are not closing it.
As a result the timing of when your client request is actually sent is unpredictable.
So, your client-side management is poor.... for a simple single-method client, you have managed to put a lot of broken things in there. I would recommend the following main-method client:
- nevermind, use 200_successes recommendation . It is good.
Given all the above, I would recommend that the socket-server management/configuration is not changed from my previous recommendation, but each client-socket-handling thread in the server should look something like:
class MyClientHandler implements Runnable {
// have one thread pool running with as many threads as there are CPU's
// this thread pool will process the actual server commands.
private static final ExecutorService COMMANDSERVICE = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
private final Socket clientSocket;
MyClientHandler(Socket client) {
clientSocket = client;
}
public void run() {
// parse the XML from the client...
try (Writer writer = new OutputStreamWriter(clientSocket.getOutputStream())) {
Document document = parseXMLDocument(clientSocket.getInputStream());
NodeList messageNodes = xmlDoc.getElementsByTagName("Command");
for (int i = 0; i < messageNodes.getLength(); i++) {
final String command = ((Element)messageNodes.item(i)).getTextContent();
writer.write(String.format("Processing command %d - %s\n", i + 1, command));
writer.flush(); // send the data to the client....
COMMANDSERVICE.submit(new Runnable() {
public void run () {
// implemented somewhere else.
// will print date, and command.
processServerCommand(command);
}
});
}
}
}
Update: The question: "My solution was to create a separate command processor thread, which would then span a new thread for each command. Any comments on my approach to ensure that commands are processed in the order that they are received?"
There are two parts to processing the commands... the first part is that they should be acknowledged to the client, and it is appropriate that the ack is in the correct order. There is no requirement that the commands are actually processed in that same order.
Notes:
- *I only just saw that the XML input file was in the question, but 'hidden'... I edited the Q and made it show up properly)
- I never reviewed this part of your problem the first time around.... it is 'bigger' than just the server-socket thread.
- the more I look in to it, the more problems I am seeing..... this is frustrating...
- your server, as it stands, does not notify the client when it receives each command, it just notifies when it receives the first data... and does not print each command....
The network server I described above is pretty typical. Here are some results from google which describe it:
- The Oracle tutorial
- another Oracle tutorial
- A Pattern/Framework for Client/Server Programming in Java
- A multi-threaded socket-based server <-- horrible code style!!!
The point is that they all follow the same pattern, and it's a good one.
Now, your problem is that a client may send more than one request. This is a problem because.... you need a protocol to support it.
- the protocol I imagined in a simple case was that the client 'dumps' the request, and the server processes it, and responds.
- if the client can send multiple requests... how do we know when one request ends, and the next one starts?
- XML input cannot just be read from the socket... because any additional client requests will cause the XML to become invalid....
- your current code just reads a file from disk, and dumps it to the socket.... and does not 'parse' the input in to individual requests. Actually, your code strips all the newlines from the input XML before sending it, so the XML is only one one line.... you should comment that, because it is not obvious.
(削除) One of the big 'gotcha's' with network processing is that data does not arrive when you expect it. For example, in your client, it may just hang forever.... because you never close the OutputStream in the client..... in fact.... **Your server WILL NOT complete the parsing of the client's data.... (削除ここまで) --- I just realized that you force all the XML on to one line, and that my previous comment is wrong... you need to document these things.
I had previously assumed that the protocol was a simple:
- client opens socket
- dumps request on socket's OutputStream
- flushes stream
- closes stream
- listens on socket's InputStream for server response.
- processes responses until InputStream is closed
- done.
But, as 200_success points out, it is nothing of the sort.... you should be forcing the data through the socket, and you are not! You are not flushing the output stream, and you are not closing it.
As a result the timing of when your client request is actually sent is unpredictable.
So, your client-side management is poor.... for a simple single-method client, you have managed to put a lot of broken things in there. I would recommend the following main-method client:
- nevermind, use 200_successes recommendation . It is good.
Given all the above, I would recommend that the socket-server management/configuration is not changed from my previous recommendation, but each client-socket-handling thread in the server should look something like:
class MyClientHandler implements Runnable {
// have one thread pool running with as many threads as there are CPU's
// this thread pool will process the actual server commands.
private static final ExecutorService COMMANDSERVICE = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
private final Socket clientSocket;
MyClientHandler(Socket client) {
clientSocket = client;
}
public void run() {
// parse the XML from the client...
try (Writer writer = new OutputStreamWriter(clientSocket.getOutputStream())) {
Document document = parseXMLDocument(clientSocket.getInputStream());
NodeList messageNodes = xmlDoc.getElementsByTagName("Command");
for (int i = 0; i < messageNodes.getLength(); i++) {
final String command = ((Element)messageNodes.item(i)).getTextContent();
writer.write(String.format("Processing command %d - %s\n", i + 1, command));
writer.flush(); // send the data to the client....
COMMANDSERVICE.submit(new Runnable() {
public void run () {
// implemented somewhere else.
// will print date, and command.
processServerCommand(command);
}
});
}
}
}
Two of the specific requirements on the server side are:
- server must run until Ctrl-c is pressed.
- it must process commends in a queue in a separate thread.
These two requirements indicate that they want a 'standard' server system, and probably a standard Executor service.
I would expect to see code that looks something like:
ServerSocket serversocket = new ServerSocket();
serversocket.bind(....);
ExecutorService threadpool = new Executors.newCachedThreadPool();
while (true) {
final Socket client = serversocket.accept();
// will need to create this class that handles the client.
// the class should implement Runnable.
Runnable clienthandler = new MyClientHandler(client);
threadpool.submit(clienthandler);
}
The above code will listen for multiple clients, not just one. It will handle each client in a separate thread.