|
I would really love to be able to specify a different SocketIOProcessor thread per Connector or ProtocolAcceptor. the common thread between all my processosr create a lot of deadlock when a connector event try to wait a reply on a different connector, blocking the whole socket processing.
My proposed change wouldn't solve your particular problem.
Although it raises a further question: Maybe the best level of abstraction for the threading model is at the SocketIoProcessor level (I.e, a SocketIoProcessor has a selector and a selector loop), but we can potentially have multiple processors. The reason I didn't do it this way in my prototype is that I wanted to keep refactoring to a minimum just to see if there was any support for the idea (and the easiest way was to bodge it in to SocketIoProcessor). If we do the thread modelling at the SocketIoProcessor level, you could then have a ProcessorFactory or whatever. This would enable any-old custom logic to map processors to SocketSessions. Hmmmm - getting more complex though, and a wider ranging change :o) Trustin? What about providing some static methods for SocketIoProcessor and DatagramConnector such as...:
* getSelectorThreads() * setSelectorThreads(int nThreads) So user can control the number of selector loops? And we could assign one SocketIoProcessor per SocketAcceptor (or SocketConnector) and make the methods above non-static. (Actually this gives a user more flexibility) WDYT? Hi,
I've been out of the office for a couple of days, and have a few meetings this afternoon. However, I'll try and take a look over the weekend, or maybe early next week - and I'll post a patch to this issue as soon as I can. Thanks, Dave Hi Dave,
Great, actually I closed Please let me know if you have any problem with this. For now, I think we can use round-robin strategy. But it would be also great if we can make it so effective. Thanks, Trustin Sounds good!
I'll try to spend some time looking at this over the weekend. Thanks, Dave I think we can make it included in 0.9.0
Users might not need to adjust the number of SocketIoProcessor in runtime. Letting user specify an appropriate number they like with the constructor will be fine for now. It would be great if there is any optimal number of processors for the number of CPUs. Any idea?
Hi Trustin,
Yes, I think this is fine. There is no real reason I can think of why you'd want to change the number of processors at runtime. The most effective number is likely to be bound in to the number of available CPUs. Certainly an easy way to configure this currently - as you say - would be to pass the number of worker threads to be employed at construction time of the SocketIoProcessor. I've got some free time today, so I'll try and tidy up the prototype and submit a patch How about creating more than one SocketIoProcessor instead of maintaining a number of worker threads? This would simplify things a lot I think. SocketAcceptorDelegate/SocketConnectorDelegate would have a list of SocketIoProcessors each. The number of SocketIoProcessors is configured using setProcessors() on the SocketSessionManager. When a new SocketSession is created the next (round-robin) SocketIoProcessor in the acceptor/connector's list of processors is passed in in the SocketSession constructor. Seems like an extremelly simple solution to me but there might be drawbacks?
Yes, that is a good and logical solution. I did think a while back that keeping the threading at the io processor level might be a good way to go (see 4th post above). And infact, the refactoring which has been done for 0.9 such that SocketIoProcessor isn't a singleton anymore makes it much easier.
Drawbacks..... The only drawback I can see is that in many systems it is most efficient to have a single pool of selector loops driving the whole IO system (as the number of threads is tied to the number of CPUs). Going this route would mean we'd have a bunch of threads for connectors and a bunch of threads for acceptors. Its not a huge problem though - just means we get slightly more context switching than we really need. The other way round would be to change SocketSessionManager to just have a setter for a ProcessorFactory or something (as in 4th post above). That way we can use the same pool across the whole of mina (acceptors and connectors) - and we dont get any code duplication as the factory does the round-robining instead of the acceptor / connector delegates. I like the idea with a SocketIoProcessorPool. Then the scheduling would be up to the pool. And you could change the pool implementation using setProcessorPool on a SocketSessionManager, right?
Yes... the interface of SocketSessionManager would change from get / set processors to setSocketIoProcessorPool.
The only problem I can see with is that SocketIoProcessor needs a link back to its parent IoSessionManager so that it can notify it of (non session related) exceptions. However, we could easily solve this by having the pool keep track of a list of exception monitors to notify when an exception occurs (i.e, the list of SSMs) Sounds good. The SocketIoProcessor constructor takes an exception monitor as an argument instead of the parent IoSessionManager. Then the SocketIoProcessorPool could be configured with an exception monitor.
Would it really be necessary to maintain a list of exception monitors? Then you would still have to make sure that an exception is passed to the correct exception monitor associated with the SessionManager the session was originally created by? Or am I missing something here? > The SocketIoProcessor constructor takes an exception monitor as an argument instead of the parent IoSessionManager
Yes > Then the SocketIoProcessorPool could be configured with an exception monitor. Possibily, but I think that is maybe forcing knowledge of too many implementation details to the user. Today, you just configure an ExceptionMonitor on an IoSessionManager (connectors / acceptors). So non-session related exceptions are provided to this monitor. I dont think pooling changes that. Sure, we now have multiple processors, but they are all working on behalf of all SessionManagers which are using them. As such, if an exception occurs in a pooled IoProcessor, I think we should notify the exception monitors of all session managers which are using the pool. I see and agree. We could use a proxy ExceptionManager which broadcasts exceptions to a set of exception managers then. No need for the SocketIoProcessor to know there are more than one ExceptionMonitor. It's important that it is a set though (and not a list) otherwise the same exception monitor could be notified multiple times about the same exception.
Yes - that sounds excellent.
So, we have: interface SocketIoProcessorPool { public void associateProcessor(IoSession s); } interface SocketSessionManager { public void setSocketIoProcessorPool(SocketIoProcessorPool pool); } SimpleServiceRegistry (and spring integration?) then sets a proxy Exception monitor on its pool. Ok, code is nearly done - just writing the plumbing in SimpleServiceRegistry to hook it all together now.
One small thing I'd like to discuss though.... Today, users create SocketConnectors directly (i.e, IoConnector connector = new SocketConnector() ). However, we'll need some way to tell the SocketConnector what processor pool it is using (we want it using the same one employed by the SimpleServiceRegistry's SocketAcceptor). SimpleServiceRegistry is nice because it hides creational logic. Likewise, when configuring with Spring integration there is also no issue. However, for non spring configuration use cases, it would be nice if the user didn't have to do anything themselves to get hooked up with the processing pool (its really an internal implementation detail). Any ideas? There could be a default singleton SocketIoProcessorPool: SocketIoProcessorPoolImpl.getDefaultInstance() which is used by all SocketIoAcceptor/Connector instances by default.
One question: why do we need associateProcessor(IoSession s)? Couldn't it just have a getIoProcessor() method? SocketIoAcceptor/SocketIoConnector would get a processor using this method and give it to the SocketSessionImpl when constructed. > There could be a default singleton SocketIoProcessorPool: SocketIoProcessorPoolImpl.getDefaultInstance()
> which is used by all SocketIoAcceptor/Connector instances by default. The only problem is that there is no "close" (or similar) in IoConnector - and users can create connectors when ever they like. Thus theres no clean way to de-register the exception monitor from the exception monitor proxy for the connector. Hmmm........ > One question: why do we need associateProcessor(IoSession s)? Couldn't it just have a getIoProcessor() > method? SocketIoAcceptor/SocketIoConnector would get a processor using this method and give it to the > SocketSessionImpl when constructed. I dont mind either way. The reason I did it this way is to avoid package dependency leak. The main interfaces (SocketSessionManager / SocketIoProcessorPool) are in socket.nio. The "how its done" (including the SocketIoProcessor class and SocketSessionImpl) is in socket.nio.support. So this way all the implementation details of the processing strategy for a session is within nio.support. Maybe a better name would be SocketIoProcessingStrategy (and maybe startForSession instead of associateProcessor). I dont mind either way though. For now a simple approach will be taken.
1) Currently, there is not much value in supplying an ExceptionMonitor per IoSessionManager. Instead, a single global ExceptionMonitor will be employed (which defaults to the "DefaultExceptionMonitor" 2) Likewise, the source of the exception is not really of any use to the user (it will be an internal Mina class). The "source" parameter will be removed from ExceptionMonitor 3) We dont need a pool interface for now. Instead, we'll add the pooling functionality straight to SocketIoProcessor. The pool size will be determined by a system property, and static utility methods will be added to SocketIoProcessor to obtain an instance to be bound to a session Hi,
I've attached a patch which I believe provides the functionality we agreed on yesterday. Cheers, Dave First attachment was empty... Sorry :o)
I checked in your patch after some modification. It works great. Thanks for the great job!
no problem at all - glad to help!
were any of the modifications serious? let me know if I got something wrong so I can make patches better in the future :o) cheers, dave Nothing serious. The only one issue I want to address is that GlobalExceptionMonitor is in 'support' package, in which users shouldn't need to access the classes. That's why I merged GlobalExceptionMonitor into ExceptionMonitor which is located in org.apache.mina.common. That's all! The patch was good. :)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Minor
Multiple selector loops
The one contentious issue is probably that I dont bother closing down "idle" worker threads once they've been opened - but that was just for the prototype really. I'll add support that back following initial feedback when I do the refactoring.