alex-vas
commented
8 years ago Note that the changes I propose, introduce a small risk of breaking current functionality. You can confirm that by studying the code. The new features are "opt in" style.
Open alex-vas opened 8 years ago
alex-vas
commented
8 years ago Note that the changes I propose, introduce a small risk of breaking current functionality. You can confirm that by studying the code. The new features are "opt in" style.
alex-vas
commented
8 years ago Added disclose_caller (supported by jawampa client) and disclose_publisher (for completeness) to the Router. The default behaviour and API is backwards compatible. It is an opt-in feature - same as in crossbar.io implementation.
This is the last change I wanted to add. Cheers.
Matthias247
commented
8 years ago Hi Alex, thanks for trying to improve jawampa. First of all I'm not opposed to add more WAMP features to jawampa I only took a short look at it (not much time, maybe more in the evening), but these are the things that I recognized so far:
threadLocalRandomClass.getMethod("current").invoke(null); is called, which I guess is slower than directly using the normal random generator. I guess a good implementation could somehow cache a random function in a static variable that is assigned once and is directly used on newRandomId(), e.g. having a static Func<Long> RandomLong; that is once initialized through reflection and does not use reflection inside. However I'm also open for just upping the java version to 1.7 if anybody can confirm/test if this still works with Android.metaApiClient is a good idea. The metaApiClient is a WampClient, which basically has it's own threading model and rules which don't conform to the WampRouters threading rules, as both implement their own EventLoop. I think you are violating the threading contracts by making direct function calls between those domains (e.g. client directly calls into router's messageReceived function), which is not allowed and will lead to race conditions. Maybe it would work if the client reuses the scheduler of the router, but it's hard to tell at the moment. There's also the lifecycle question: These clients are created when the router is created but they are never stopped, so they will leak around when the router or realm is closed. I think some proper in-process client implementation would require a solid implementation of a WampTransport that uses a thread safe queue to transport messages from a client to a router. For creating a client in the router more likely a special implementation of the WampClient would be needed that does not perform any implicit queuing and would directly live in the Routers scope. But these all would be some quite big changes. To just implement these features I would more likely create a helper function on the router that publishes an event to all subscribed clients (move the dispatch event handling from PublishMessage handling to there) and just call that method when a client joins or leaves or when a PublishMessage arrives. Maybe the publishEvent method can and should be changed to support this and to don't take a PublishMessage as an input parameter, but this can be quite simply explored.alex-vas
commented
8 years ago Hi Matthias,
Thank you for your reply.
This pull request contains the implementation of several features/improvements (e.g. random generator, disclose api, session api). I know it's convenient to work like this, but reviewing and merging individual things gets hard. Please try to seperate them in future.
Apogees for that. You are right about random generator. It is a complete separate piece. Disclose API is separate too. I guess I need to make a separate Github fork for those isolated changes.
The session API is based on in-process-client feature, so those are harder to separate. Not sure if Github can help me with it, OR if I need to make a pull request for in-process-client, then wait for it to be merged to your trunk, and only then create another pull request for session API...
Anyway, thanks for pointing that out, I will try to do a better job next time.
The feature announcement for the session api is missing. Maybe also for disclose feature, haven't looked at that yet.
There few changes seemingly missed being checked in. Apologies, this should be fixed now.
The new random implementation does use reflection for generating each random number. For all of those threadLocalRandomClass.getMethod("current").invoke(null); is called, which I guess is slower than directly using the normal random generator.
Not sure about it, but I understand that a reflection call could make it feel uncomfortable from the performance perspective. The problem with the original Random class is that it enters a critical section on every call. It is expensive and scales badly with the number of CPU cores.
I guess a good implementation could somehow cache a random function in a static variable that is assigned once and is directly used on newRandomId(), e.g. having a static Func
RandomLong; that is once initialized through reflection and does not use reflection inside.
You talking about generating the compiled class dynamically? Like with cglib? I would say that it is too hard for what we are trying to achieve here.
However I'm also open for just upping the java version to 1.7 if anybody can confirm/test if this still works with Android.
The problem here is that Android Runtime (ART) is not aligned to Java ME or Java SE standards. So, by lowering runtime compatibility, we are only improving our chance of not using a class which is not implemented on android. For example, Swing classes are not implemented on android entirely, but available in Java since 1.2. As for the source compatibility, sounds like 1.6 is safer choice (see https://developer.android.com/studio/tools/sdk/eclipse-adt.html)
But, yes, we need an Android developer to confirm (I am not).
Session API: Yes, I think it would be good to have that implemented. But I'm not convinced that implementing it with this metaApiClient is a good idea. The metaApiClient is a WampClient, which basically has it's own threading model and rules which don't conform to the WampRouters threading rules, as both implement their own EventLoop. I think you are violating the threading contracts by making direct function calls between those domains (e.g. client directly calls into router's messageReceived function), which is not allowed and will lead to race conditions. Maybe it would work if the client reuses the scheduler of the router, but it's hard to tell at the moment. There's also the lifecycle question: These clients are created when the router is created but they are never stopped, so they will leak around when the router or realm is closed.
The caller would assume responsibility to close them. In case of meta session API, yes, you are right, I should make sure we close them. ...One sec... FIXED.
I think some proper in-process client implementation would require a solid implementation of a WampTransport that uses a thread safe queue to transport messages from a client to a router.
I can look into using a thread safe queue, but I am a bit confused as you are talking about a different approach below:
For creating a client in the router more likely a special implementation of the WampClient would be needed that does not perform any implicit queuing and would directly live in the Routers scope.
I know what you are saying, I have attempted to do that at first, but then realized that it would require some significant change to the architecture and would more likely introduce some regressions, therefore I changed to the plan "B", the one I have implemented.
But these all would be some quite big changes. To just implement these features I would more likely create a helper function on the router that publishes an event to all subscribed clients (move the dispatch event handling from PublishMessage handling to there) and just call that method when a client joins or leaves or when a PublishMessage arrives. Maybe the publishEvent method can and should be changed to support this and to don't take a PublishMessage as an input parameter, but this can be quite simply explored.
Again, quite a significant change. I am concerned that if I would implement a change of this scope you might feel uncomfortable merging it in. If I would start implementing it, I feel that I need to break WampRouter class down to smaller pieces, so it would remain manageable. This might, in turn, break the library API compatibility...
The last problem with the above change is that it would be harder for me to justify it in my organization. I have family and kids, and I cannot afford implementing it at my spare time. Those times, when I was doing those type of things, long gone now. ;) So I am doing it at my work time and my company pays for it. Though, thankfully my company kindly agreed to spend some time working on an open source project this time, when we were needed the feature, I would have much harder time trying to convince them that I need to spend much more time to make a more proper implementation. Unfortunately.
Would you consider implementing createInProcessClient() for the WampRouter? And also the "helper function on the router that publishes an event to all subscribed clients"? Then I can build all the other changes on top of it. I would not have problems with it.
Correct me if I am wrong, but the server seemingly running single threaded. Any plans to make it multithreaded? I might have to look into it too - some time later...
Cheers, Alex
There are two features in this pull proposal: one feature is an ability to create in process clients, which are connected to the given router:
Affectively now you can subscribe to "wamp.session.on_leave" channel and get notified when any other clients leaves (disconnects) from the router.
Two channels are supported: "wamp.session.on_join" and "wamp.session.on_leave".
Fell free to criticise any part of the change or whole. I am happy to incorporate any feedback especially quality related.
Cheers, Alex