|
Please look at
Your fix solves the problem. Thanks.
My rationale behind having filterEnabled() and filterDisabled() was to have something like a constructor/destructor for a filter. The filter programmer can do all setup/teardown work in the respective callbacks. Even if the filter gets applied to an Acceptor, filterEnabled() would get called only once for its lifetime and not for each accepted connection. This helps a lot in having a clean filter implementation IMO. There was init() and destroy() method long time ago. But I removed them because a filter instances can be used by more than one session managers. For example, thread pool filters could be shared by all acceptors and connectors. filter chains will have to maintain the glocal reference count of filters to call init() and destroy() only once for each. Do you agree?
The SSL/TLS filter needs to do a handshake when it is applied to a session and also may need to discard sensitive data when it is being removed. Currently this is not possible as isolated activities because if the filter is applied on an active session, the handshake process will not start unless any data is sent/received over the session.
Your previous comment also reminded me about something -The definition of a "Filter". For me, a filter is something that taps the session and monitors/modifies the data flowing over it. I've not been able to digest a ThreadPoolFilter. Thats actually an implementation of a concurrency mechanism rather than a Filter (according to my definition). Maybe if we separate it out into a separate entity, the problem with init() and destroy() might also be solved? I'll have to check on the current implementation to grok what u're talking abt (filter chain reference counting et al.). Will do that and get back to you on other possible alternatives. This fix doesn't work in required circumstances.
Basically this depends on one of the endpoints writing some data on to the session to start the ssl/tls handshake process. According to the SSL/TLS RFC, the client initiates the handshake process. If the handshake fails for some reason, the connection is typically terminated. In the case of the MINA SSLFilter, the handshake can begin only when one of the entities writes data to the session. And if the server writes data first, that data is never sent across since the handshake is pending. In other words, it becomes imperative that the client send some data first. If the application protocol mandates that the server first write some data, the MINA SSLFilter won't work. The only good fix would be to move the handshake process into an init() method that would get called automatically when the filter is applied. destroy() should also be provided to remove any sensitive information that is stored. Also, the filter should have a callback mechanism by way of which it can notify the handler that the initialization process is complete and actual data transmission can take place. This would be required for proper error handling in the application protocol. I added init() and destroy() callback to IoFilter. They are automatically called when a filter is added to or removed from filter chain. AbstractIoFilterChain counts reference count of filters to prevent chain from calling init() or destroy() multiple times in case that filters are added more than once to different chain.
init() and destroy() without any input parameters are not useful esp for filters such as the SSLFilter.
Taking the example of SSLFilter, an instance of SSLHandler should be logically created in init() if the filter is being applied on a Session that is already active. But in this case, since no Session object is being passed, the filter cant do anything. Also, what should be passed in init() when the filter is being applied on an Acceptor? I replaced ini() and destroy() with filterAdded() and filterRemoved() as you requested. Please check my changes in the trunk. It now passes IoSession or IoSessionManager as a parameter whenever it is added to them. :)
Please close this issue after reviewing it. SSLFilter will need to be updated to use filterAdded and filterRemoved.
I've renamed IoFilter.filterAdded() and filterRemoved() to init() and destroy() to avoid name confusion.
I changed IoFilter more while revolsing
http://svn.apache.org/viewcvs.cgi/directory/network/trunk/src/java/org/apache/mina/common/IoFilter.java?rev=350169&view=markup |
|||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Blocker
Filter API needs callback for enabled notification
FilterEnabled() and filterDisabled() is a good idea, but adding a filter to an acceptor causes some problem:
* A lot of events are fired at once to cause overload
* SessionManagers have to manage the list of sessions, and we have to handle concurrecy issues while iterating session list.
I think it is best for filter developers to check any data associated with session is initialized before filtering IMHO.