Thanks for all the great points Nate. I will definitely make some changes based on your comments.
I took a peek, though while I've written some NIO stuff, I don't pretend to be an expert.
* You allocate 8KB for every read. You might want to allocate one buffer per client. Also you may want to use a direct buffer, but I haven't personally tested that it is better.
Each client gets and instance of Client. Since I allocate 8KB inside the client, that is only 1 buffer per client. It shouldn't hurt to use direct ByteBuffers so I can make that change.
* Client#run has @Override but doesn't, so doesn't compile for me. Same with Main#run().
Both Client and Main implement Runnable and therefore override public void run(). It was Eclipse that put those annotations in. I usually don't remove them because it doesn't cause me a problem.
* In Client#read()... Not sure you need the "channel.isOpen()"? There is no guarantee the channel is still open inside the IF, since it can go bad at any time.
Most of the examples I found used that so I thought it was appropriate.
* In Client#read() you use "new String(byte[])", I'm guessing you probably want to explicitly specify a charset?
True.
* "messageBufferInUse" looks suspicious. You synchronize on "lock" to set it true and false, but in Client#run you read its value without the lock. It looks like you are trying to avoid the synchronize and sleep instead, but it would be better to just synchronize and your thread will wake up as soon as the monitor is free.
* In Client... You copy the new string into a StringBuffer which is thread safe, and you also do the locking yourself. You probably want to use StringBuilder and lock yourself, of StringBuffer and let it do the locking.
Yeah, I didn't quite get that right. I will have to fix it.
* Not sure why you start a new Thread in the Client constructor? It seems you were going well with one thread, then fell back to the good old ways when it came to the DataConnection stuff.
I was thinking that since a command may not come through completely in on read I would need to keep checking the message buffer. However I thought of another way that will remove the thread from client and the whole thing can be in one thread.
* With NIO, after connect, generally you should only have OP_READ registered. When you want to write something, you should just try to write it. This almost always succeeds, and avoids the overhead of OP_WRITE. If not all the data could be written, only then should you register both OP_READ and OP_WRITE to write the remainder. When all the data you have is written, you should go back to just OP_READ.
Currently you seem to have OP_WRITE registered all the time. This will introduce unnecessary delays because your data won't be written until the next select. For FTP this is probably negligible. However, selector.select() in Main#run will always return immediately since it thinks you have data to write, and Client#write will be called repeatedly when there is no data to write. You effectively have a "busy wait".
I didn't know that. Thanks.
Thanks again Nate.
