I have a subclassed Thread with a private Selector and a public register(SelectableChannel channel, ...) method that allows other threads to register channels to the selector.
As answered here, the channel's register() blocks during selector's select() / select(long timeout) so we need to wakeup() the selector.
My thread selects indefinitely (unless it gets interrupted) and it actually manages to get into the next select before channel's register() is called. So I thought I use a simple lock with synchronized blocks to ensure the register() happens first.
The code: (irrelevant code removed for readability)
public class SelectorThread extends Thread {
...
public void register(SelectableChannel channel, Attachment attachment) throws IOException {
channel.configureBlocking(false);
synchronized (this) { // LOCKING OCCURS HERE
selector.wakeup();
channel.register(selector,
SelectionKey.OP_READ,
attachment);
}
}
@Override
public void run() {
int ready;
Set<SelectionKey> readyKeys;
while (!isInterrupted()) {
synchronized (this) {} // LOCKING OCCURS HERE
try {
ready = selector.select(5000);
} catch (IOException e) {
e.printStackTrace();
continue;
}
if (ready == 0) {
continue;
}
readyKeys = selector.selectedKeys();
for (SelectionKey key : readyKeys) {
readyKeys.remove(key);
if (!key.isValid()) {
continue;
}
if (key.isReadable()) {
...
}
}
}
}
}
This simple lock allows register() to happen before the thread continues with the next select loop. As far as I tested, this works as supposed.
Questions: Is that a "good" way to do it or are there any serious downsides to that? Would it be better to use a List or Queue (as suggested here) to store channels for registration, or a more sophisticated lock like this instead? What would the Pros/Cons of that be? Or are there any "even better" ways?
I am actually surprised the lock acquisition with the empty block is not removed at compile time. Pretty cool that it works. I mean it works, it's preemptive, it's not the prettiest approach, but it works. It's better than a sleep as it is predictable and since you use the wakeup call you know progress will be made as needed rather than on a periodic update if you purely relied on the select timeout.
The main downside of this approach is that you are saying that calls to register trump anything else, even servicing requests. Which may be true in your system, but usually this is not the case, I would say this is a possible issue. A minor issue which is more forward thinking is that you lock on the SelectorThread itself which is sort of a larger object in this case. Not bad, not great though as you expand, this lock will just have to documented and taken into account whenever other clients use this class. Personally I would go with making another lock altogether to avoid any unforeseen future hazards.
Personally, I like the queuing techniques. They assign roles to your threads, like a master and workers this way. Whereas all types of control happen on the master, like after every select check for more registrations from a queue, clear and farm out any read tasks, handler any changes in the overall connection setup (disconnects etc)... The "bs" concurrency model seems to accept this model pretty well and it's a pretty standard model. I don't think it's a bad thing as it makes the code a bit less hacky, more testable, and easier to read imo. Just takes a little more time to write out.
While I will admit, it has been a long time since I last wrote this stuff, there are other libraries out there that sort of take care of the queueing for you.
Grizzly Nio Framework while a little old, last time I used it, the main runloop was not bad. It setup a lot of the queuing for you.
Apache Mina Similar in that it provides a queuing framework.
But I mean in the end it depends on what you are working on.
Unless you are planning on using this as a core piece of a service you are providing to customers, I would say your approach is fine. It may just have maintenance issues in the long run.