|
I think you're right on the issue with sessions being opened / closed. Main thing is that only the higher level protocol (on top of UDP) can really determine the meaning of a
'session' in its very own context. It might be possible to design some kind of protocol filter which implements generic session tracking on top of simple message delivery by: - tracking received message from the remote peer and fire off a sessionOpened() event BEFORE passing the message onto the next filter in the chain - implement a timeout-based mechanism that tracks the data messages passing through and closes the session if the timeout passes by (including firing the event) But it does not seem feasible to implement this as a generic behauviour of the datagram acceptor (as there are protocol which do not have the concept of a session at all) But I do think that it is a bug in the implementation of the datagram acceptor if it does not assembly the filter chains when the individual session get create. I understand that it is currently needed ot create a session per received message but it should then assemble the filter chain per session as one should expect from the documentation. It would then be necessary to document this as an user might need to take special precautions in this chain builder implementation. This is a great idea. We could provide a generic session tracking strategy interface so users can choose their favorite session management method. For instance:
/** A kind of cache? */ public interface ConnectionlessSessionTracker { /** * @return null if no session is found */ IoSession getSession( SocketAddress localAddress, SocketAddress remoteAddress ); } WDYT? I already approached this but I found out that it will not work without a acceptor facade which encapsulates the "real" datagram acceptor and does the session tarcking on behalf of
the client. This thing is in fact second-next on my to-do list for my current project. Once this facade works, it can easily implement the proposed tracker interface. If it works, I will supply my implementation as a diff to mina-0.9.0 Your idea sounds very good. Did you implement anything related with this?
Yup. I have a working IoAcceptor, the IoConnector is planned but I didn't have time so far. I still need to polish the session timeout detection code but it works so far. Hopefully I can get this finished over the weekend.
Here's my first stab at fixing this. Could somebody please review it? I haven't touched the MINA code before just tonight :)
The attached diff is against svn revision 440993, the current trunk at this time. Basically, you can implement the methods void sessionCreated(IoSession session); IoSession getSession(SocketAddress localAddress, SocketAddress remoteAddress); in a ConnectionlessSessionTracker, and assign that to a DatagramSessionConfig. From then on, the tracker will perform session management for the transport. The only current problem is that the session cannot be retrieved until the remote socket address is known. In DatagramChannel, this happen upon read, so the session cannot be queried for its read buffer size. I think it should still be the same, but once again, please review it. Here is also a testing version of a ConnectionlessSessionTracker (don't use, it doesn't expire or do anything advanced enough): import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.HashMap; import java.util.Map; import org.apache.mina.common.ConnectionlessSessionTracker; import org.apache.mina.common.IoSession; public class TestSessionTracker implements ConnectionlessSessionTracker { private Map<String, IoSession> sessionMap = new HashMap<String, IoSession>(); public IoSession getSession(SocketAddress localAddress, SocketAddress remoteAddress) { InetSocketAddress localIsa = (InetSocketAddress) localAddress; InetSocketAddress remoteIsa = (InetSocketAddress) remoteAddress; String key = new StringBuilder(localIsa.toString()).append(remoteIsa.toString()).toString(); return sessionMap.get(key); } public void sessionCreated(IoSession session) { InetSocketAddress localIsa = (InetSocketAddress) session.getLocalAddress(); InetSocketAddress remoteIsa = (InetSocketAddress) session.getRemoteAddress(); String key = new StringBuilder(localIsa.toString()).append(remoteIsa.toString()).toString(); if (!sessionMap.containsKey(key)) { sessionMap.put(key, session); } } } I imagine that by integrating iofilters that know of the session tracker, etc you could implement just about any session management strategy. Let me know what you think! Your patch is great generally, but needs subtle improvement:
1. We have to decide where to put get/setSessionTracker(). I thought puting it in IoServiceConfig level would be better. If this is right, we will need ConnectionlessIoServiceConfig interface. 2. We also need to be notified when a user calls IoSession.close() to prevent memory leak. 3. Connectionless is just too loooooong. Wouldn't there be any better name? I think this issue should be fixed in 1.0.
>Your patch is great generally, but needs subtle improvement:
Thanks! I expected as much, it is just a first stab. >1. We have to decide where to put get/setSessionTracker(). I thought puting it in IoServiceConfig level would be better. If this is right, we will need ConnectionlessIoServiceConfig interface. I also initally thought this, but could not figure out how to access the current IoServiceConfig from the relevant areas in DatagramAcceptor/Connector(Delegate) without some major refactoring ... maybe you can provide some hints on that... >2. We also need to be notified when a user calls IoSession.close() to prevent memory leak. I initially thought this could be left up to a filter (similarly, some protocols may implement certain messages (like a bye message) that can close a session), but it does seem like a good thing to go ahead and integrate into the interface. >3. Connectionless is just too loooooong. Wouldn't there be any better name? Hehe .. but isn't that what Java is about? :) Well, how about just SessionTracker? Perhaps there are even protocols out there whose sessions span TCP connections? Maybe this would be great as an optional behavior for any IO service. .>I think this issue should be fixed in 1.0. That would be awesome! Sorry to double post, but I also wonder what do about this (mentioned in previous message):
The only current problem is that the session cannot be retrieved until the remote socket address is known. In DatagramChannel, this happen upon read, so the session cannot be queried for its appropriate read buffer size. 1. Once you have an access to an IoSession, then you can get an appropriate IoServiceConfig by calling IoSession.getServiceConfig(), which is introduced in 1.0-SNAPSHOT.
2. Adding a filter to do this is not a good idea because an event can be bypassed by other filter. 3. Sounds very nice. What about this: * ConnectionlessSessionTracker -> IoSessionRecycler (I think this is a better name.) * Add IoServiceConfig.get/setSessionRecycler() and document that this is for connectionless transports only. Ah I see. Then you will have to use RegistrationRequest. It contains IoServiceConfig.
I did reply on this issue page, it's a few comments above, I guess I fooled you with my '>' marks :)
1. OK, I did not notice that method on IoSession when I checked. That is great! 2. I agree, since IoSession.close() is MINA's method anyway, we should integrate 'sessionClosed(IoSession)' into the interface. Users will still want to add an 'early' filter (probably right after ThreadPoolFilter) to detect things like BYE messages that close sessions, though (I think). 3. IoSessionRecycler is a very nice name. I agree, IoServiceConfig is the right place. Do you also want to allow this optional session recycling for transports with connections? What do you think about the read buffer size issue I mentioned before? Oh, hehe. I deleted my duplicate reply. You are right. We need sessionDestroyed(IoSession) there, too. Probably it's not an event handler but a recycler, we might need different names. Here's my suggestion:
* IoSessionRecycler * void put(IoSession); * IoSession recycle(SocketAddress localAddress, SocketAddress remoteAddress); * void remove(IoSession); For now, I don't have any idea on recycling a session with transports with connections. Do you have any idea? And at last but least, we need a default implementation. We could provide the default implementation with timeout. A session, which is not being recycled for a certain amount of time (in seconds). could be removed and sessionClosed() event could be fired automatically. Attached is the next version with IoSessionRecycler per Trustin's and my comments. I still am not sure about the read buffer size issue (marked by a TODO in the code). Also, after some thought I don't think this is applicable to transports with connections (seems like too much abstraction, and doesn't fit well with the MINA session paradigm).
I also added a default implementation, ExpiringSessionRecycler, which uses an ExpiringMap to recycle sessions. It also calls sessionClosed on the session's filterchain when a session expires. I hope this helps! I definitely want to contribute something nice that will speed MINA along to 1.0 :) Let me know what you think! Also, I could not find a good implementation of an expiring map, so I wrote my own. Please review that code carefully since I wrote it fairly quickly. It uses a timed expiry thread set at low priority.
Minor change to build filter chain only when a session isn't recycled (save some clocks :)
The patch is great! I will apply it.
Regarding the read buffer size, we have to refactor! :) Great! I'm happy I could help. I'll leave the refactoring to you, since you have a better view of the high level stuff. I guess the Expiry thread will need to be run by the Executor now, per Peter's e-mail.
If I get a chance, I'll take a closer look at DatagramAcceptor/Connector and see if there's anything else I can think of doing. Fixed finally. Thanks to Greg and Rainer for deep discussion and patch! :)
I just refactored my work based on the latest revision.
Firstly, I used the backport concurrent classes to achieve thread safety in the ExpiringMap. Also, I had the idea to model ExpiringSessionRecycler after ExecutorThreadModel, and have ExpiringSessionRecyclers associated to services. Next, I created IoSessionRecycler.NOOP (doesn't ever recycle a session), which is set to be the default IoSessionRecycler implementation since I don't think we can assume that people want session recycling on by default. It also saves them a thread if they don't, since otherwise one would be created for the Expirer no matter what (since ExpiringSessionRecycler was default implementation). Lastly, I made the Expirer thread a daemon thread to prevent holding up a shutdown process. so if you want recycling, do something like: ExpiringSessionRecycler recycler = ExpiringSessionRecycler.getInstance("serviceName"); recycler.startExpiring(); serviceConfig.setSessionRecycler(recycler); Let me know what you think! I always forget something after I hit 'submit'.
In ExpiringSessionRecycler, I liked your idea of using the List as the key instead of the concatenated String representation. I must have been on crack when I did it that way :) I also saw your comments about IdentityHashMap. I don't think returned SocketAddresses from the channels are guaranteed to be reference equal? I think ConcurrentHashMap (used in my v4 patch) is a good middle ground because it tries to avoid any synchronization (as much as possible) for simple reads (which, if the user wants recycling, will probably be the vast majority of calls) and only synchronizes when a change to the map is in progress. Just my $0.02. (I have a lot of 'em, eh?) Thanks for keeping up with this Trustin, and I look forward to your feedback. And, in an astounding extra $0.02, I also wanted to remind you to update the IoHandler javadoc since it indicates that sessionOpened/etc will not be called for UDP.
I'd also like to put out there that sessionCreated could be called on the session's filter chain when it is first created (before sessionOpened), this would make all MINA events applicable to the UDP transport. I know it doesn't really mean anything different, but it might be the least surprise to the user. And on that note, I'm going to go do something else for a while so I don't think of anything else to spam the mailing list with updates about :) Actually works with the test case now. Also, I cleaned up some Windows CRLFs in the test case file.
Greg, thank you again! :)
1. The reason I talked about IdentityHashMap was just about the case when a key is an IoSession. We have to use HashMap for SocketAddresses, of course. 2. Oh yeah I have to update IoHandler. 3. I think ExpiringSessionRecycler should be the default recycler because NOOP can cause a lot of overhead. 4. startExpiring() could be called automatically at the first invocation to put(). Everything's fine except that! If you are willing to submit the newer patch, then you could update IoHandler documentation and take care of other stuff I mentioned in this comment. Please let me know if not, then I will take care of it. :D No problem, happy to lend a hand :) MINA makes my work easier, so it's more than a fair trade.
I can get to patching the remaining stuff (IoHandler, defaulting to ExpiringSessionRecycler) within the next couple of days or so. I agree that ExpiringSessionRecycler should be the default now that I think about all the events flying around the filter chain with NOOP, and starting the expiration after the first put is a good compromise. Nice to have util-concurrent for that, since a double check lock is actually thread-safe with ReentrantReadWriteLock. Do you want me to add an IdentityHashMap for session lookup? It would mean extra synchronization, and it would affect only the 'remove(IoSession)' method AFAIK. Are the costs to generate the key high enough to justify that? Updated per Trustin's suggestions.
Yargh. In this one (v7), I do the lock upgrade correctly in the startExpiringIfNotStarted method of ExpiringMap. Sorry 'bout that. Somebody may want to refactor that state locking code to some common class, cause it would minimize the chances to create bugs like that :)
Hi Greg, thanks for the patch again! I applied it successfully, but I'd like to mention a few tiny problems:
* Please don't auto-format the code. It makes me really hard to review the patch. * Please run all unit tests before submitting a patch. I found the test fails due to a bug in ExpiringMap. It was a trivial one, so I fixed it fairly easily. Except that, it's very good. Here's the list of modifications I made on top of your patch: * Changed the type of TTL and expiration interval to integer. * ExpirationSessionRecycler doesn't implement ExpirationListener directly. I made an inner class instead. * Removed the static getInstance() method in ExpiringSessionRecycler because we are already using a static global expirer by default. We need some documentation on creating many recyclers though. Thank you again! Sorry about that! I tried my best to resist the format keystroke in Eclipse, but I must have slipped up. I know how annoying that can be.
I did run the unit tests successfully before my v7, but not before the v6. My mistake. Was the bug due to upgrading the lock incorrectly as I fixed in v7, or was it another one? Good ideas on the modifications. I hope I at least saved you a little time in getting this implemented. I think the v7 version works for locking on startExpiringIfNotStarted, at least when running the test case here.
The SVN version of startExpiringIfNotStarted seems almost ok, but I think you leave a read lock locked in the case that the expirer is not already running. Trustin, not sure if you overlooked my last comment. I think you might be leaving a read lock locked in ExpiringMap.startExpiringIfNotStarted() if the expirer hasn't started yet.
Just wanted to catch your attention before 1.0 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Minor
datagram session management fails
We also cannot maintain all IoSession instances because we don't know when it becomes inactive. Perhaps we could give connectionless sessions some kind of 'lease' (timeout) to workaround this issue. If there's no I/O for a certain period, the sessionClosed() event will be fired and the session will be disposed. What do you think?