Patch for shared sessions (multiple browser tabs)

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Open the same GWTEventService application in two different tabs of the 
same browser
2. Watch the events

What is the expected output? What do you see instead?
Events are only received by one application.

What version of the product are you using? On what operating system?
GWTEventService 1.1, on Fedora 11

Please provide any additional information below.

GWTEventService 1.1 uses the http session ID as a clientId, thus preventing 
multiple clients from the same session from working propely.  

This patch requires the servlet path in web.xml to be modified from 
<something>/gwteventservice to <something>/gwteventservice/*.  The extra 
URL component is used to pass in an "app id" which identifies each browser 
tab.

GWTRemoteEventConnector is responsible for generating the app id at 
startup, and constructing a URL which uses it.  EventServiceImpl now uses 
the app id as well as the session id to construct the clientId, both when 
registering on a domain and when listening to a domain.

Note: the method 
EventExecutorServiceFactory.getEventExecutorService(HttpSession) is not 
really compatible with the idea of shared sessions, since a single session 
may support multiple clients.  I'm not sure of the best solution here, 
perhaps deprecation?  (I don't use this method myself.)

Original issue reported on code.google.com by sean.flanigan@gmail.com on 25 Mar 2010 at 8:03

Attachments:

• gwteventservice-shared-session-support.patch

[GoogleCodeExporter]

I've just been testing my patch with HelloGWTEventService and 
DemoConversationAppUI in 
DevMode, and so I had to change the web.xml servlet path as mentioned above. I 
also had 
to modify GWTEventService.gwt.xml to get rid of a warning from GWT.  
Supplementary 
patch is attached.

Original comment by sean.flanigan@gmail.com on 25 Mar 2010 at 1:11

Attachments:

• web.xml.patch

[GoogleCodeExporter]

Wow! That is a very important part for GWTEventService 1.2 and my thought about 
a
solution to solve that made me crazy. ;-) I hadn't thought that it is solvable 
with
so few lines of code. Thank you very much for that patch. I will integrate it 
at the
weekend with small code style modifications.

The streaming implementation is also nearly done. Not that we have two 
solutions for
streaming next week... ;-)

Original comment by sven.strohschein@googlemail.com on 25 Mar 2010 at 5:46

• Changed state: Accepted

[GoogleCodeExporter]

Hi,

I have tested the patch today and the most is working nice, but I need to do 
some
modifications to solve the following problems and the integration will be a 
little
bit delayed.

- EventFilter aren't working anymore probable because the session is needed 
somewhere
- com.google.gwt.user.client.Random class should be used instead of 
java.util.Random
to prevent compatibility issues
- a solution for 
EventExecutorServiceFactory.getEventExecutorService(HttpSession) is
needed

I will do these modifications in the next days and your patch helps me a lot! 
How do
you have realized it first, without the web.xml modification? :-)

Thanks again for your patch!

Original comment by sven.strohschein@googlemail.com on 28 Mar 2010 at 12:32

[GoogleCodeExporter]

- the client id generation should be extracted with a configurable factory

Original comment by sven.strohschein@googlemail.com on 28 Mar 2010 at 12:36

[GoogleCodeExporter]

Doesn't GWT replace java.util.Random with com.google.gwt.user.client.Random at 
compilation time anyway, or did I just assume that?  Or are you trying to make 
sure no-
one uses one of the methods which aren't available in GWT's version?

I originally used a random UUID instead of Random.nextInt, but it shouldn't 
make any 
difference.  I think it should be safe (even if there were a collision, it 
could only 
collide within the same Session).  Alternatively, the server could be 
responsible for 
generating incremental clientIDs and passing them back to clients.  You've 
still got to 
trust the client though, so it's probably not worth the effort.  (And your idea 
of a 
ClientIdFactory certainly makes sense.)

Perhaps EventExecutorServiceFactory.getEventExecutorService(HttpSession) should 
return 
a List<EventExecutorService> for each client using that Session?

As for my testing, I originally tested against my own application and modified 
its 
web.xml, that's all!

Original comment by sean.flanigan@gmail.com on 28 Mar 2010 at 12:52

[GoogleCodeExporter]

Are you sure, that a collision can not occur? The client id is attached to the
servlet path and parsed on the server side without checking the session or not?

I have also thought if it is better to generate the id on the server side. 
There is
already an init call to transfer the configuration to the client side. The init 
call
could also contain the client id. The problem is, that the service 
initialization
(where the servlet path is extended) must be re-initialized. The other way 
would make
problems, because a separated client configration for the ClientIdFactory is 
required
instead we have the same problem with the re-initialization. I think to use the
server side way, is the better option.

Original comment by sven.strohschein@googlemail.com on 28 Mar 2010 at 1:35

[GoogleCodeExporter]

Well, a collision could theoretically occur (if the random numbers aren't very 
random), but my patch constructs the client ID by concatenating the session ID 
with 
the random discriminator, so any such collisions would be restricted to the 
same 
session.

The client doesn't actually pass the entire client ID to the server, just the 
discriminator.  The server always constructs the client ID by combining the 
session ID 
and the discriminator.

Original comment by sean.flanigan@gmail.com on 28 Mar 2010 at 11:53

[GoogleCodeExporter]

Ah, you are right, that was a good idea for the realization! :-)

I have already committed some preparing modifications for the configuration. 
The rest
can be committed in the next days.

Original comment by sven.strohschein@googlemail.com on 31 Mar 2010 at 2:37

[GoogleCodeExporter]

Hi,

it is nearly done and could be committed. It is working with normal adding of 
events
to a domain and listening, but we have detected a problem with adding 
user-specific
events and user-specific EventFilters. You have also detected it with
EventExecutorServiceFactory.getEventExecutorService(HttpSession). That can't be
deprecated, because RemoteEventServiceServlet uses it, too. When
addEventUserSpecific(...) or setEventFilter(...) is called, the client / 
session id
of the calling client is taken by default. addEventUserSpecific shouldn't be
critical, because it is a little bit senseless when it is used in that way (the
calling client adds an event only for itself), but to change setEventFilter is
critical. For example the DemoConversationApp uses it in
ConversationServiceImpl#join(...) and I think it isn't easy to let it work with
another id.

The problem is, that GWTEventService hasn't control about the servlets which 
extend
from RemoteEventServiceServlet or which are using the EventExecutorService. 
Therefore
we are not able to deliver an own id with a request.

At the moment I'm thinking about a solution via an additional client service for
adding events. That was planned for GWTEventService 1.2, anyway. With that 
service we
have control about the calls and can provide the client id. That way is only 
required
for addEventUserSpecific and setEventFilter/getEventFilter/removeEventFilter.
Anything else (the most cases) can still be done via the server side. That can't
actually resolve in a disadvantage, because calls which aren't migrated to the 
client
service would cause the same problems as before, when more than one browser tab 
is used.

Do you (or someone else) have a better solution or a better idea to solve that?

Original comment by sven.strohschein@googlemail.com on 2 Apr 2010 at 6:04

[GoogleCodeExporter]

I finally had time to think about it, and came up with an idea, completely 
untested so far.

EventExecutorServiceFactory.getEventExecutorService(HttpSession) can only 
really work with 
one client per session, but what about modifying RemoteEventServiceServlet to 
add a similar 
getClientId() method:

    private EventExecutorService getEventExecutorService() {
        final EventExecutorServiceFactory theEventExecutorServiceFactory = 
EventExecutorServiceFactory.getInstance();
        return theEventExecutorServiceFactory.getEventExecutorService(getClientId(true));
    }
    private static boolean IGNORE_APPID = false;
    protected String getClientId(boolean isInitSession) {
        HttpServletRequest request = getThreadLocalRequest();
        if (request == null)
            return null;
        final String sessionId = request.getSession(isInitSession).getId();
        if (IGNORE_APPID) {
            return sessionId;
        } else {
            String path = request.getPathInfo();
            if (path == null)
                return sessionId;
            int lastSlash = path.lastIndexOf('/');
            String appid = path.substring(lastSlash+1);
            return sessionId + "-" + appid;
        }       
    }

(Sorry this isn't in a patch, but this is more of an idea than a compilable 
patch right 
now.  And I'm not sure how much of the previous patch is already merged into 
trunk.)

Perhaps both servlets should extend a common parent so that they can share the 
implementation of getClientId().

As for testing it:  EventExecutorServiceFactoryTest will need to construct a 
mock 
HttpServletRequest and get it into threadLocalRequest somehow.  And I guess 
DemoConversationApp would provide a more concrete test, if you have time to try 
it.  (I 
haven't had a chance yet.)

All of this should allow a RemoteEventServiceServlet to use the new URL scheme 
for 
distinguishing between app ids, *if* it chooses to.  If not, it will just fall 
back to the 
old behaviour of one client per session.  (At least, that's the plan.)  This is 
the same as 
with the other servlet.

By the way, I noticed that the JavaDocs for 
EventExecutorServiceFactory.getEventExecutorService(HttpSession/String) say 
that they 
return a singleton per session, but the unit test 
(EventExecutorServiceFactoryTest) 
actually checks that the instances are different, which they are.  Which is 
correct?

Original comment by sean.flanigan@gmail.com on 28 May 2010 at 3:09

[GoogleCodeExporter]

I have committed all changes to support mulitple / shared sessions, now. It can 
be
activated with the property
"eventservice.connection.id.generator=de.novanic.eventservice.service.connection
.id.SessionExtendedConnectionIdGenerator".
I have also found a solution to support it without changing the mappings in the
web.xml and without to parse the path elements manually. All you have to do is 
to
configure the other ConnectionIdGenerator with the property described above. 
You can
take a look at it in the trunk version (SVN). That logic is working with pure 
session
ids (default and like before) and with the new "extended" sessions, but 
user-specific
events and event filters are still not working properly when that
ConnectionIdGenerator is configured. That's why it isn't in the default 
configuration.

The next step is the client interface to user-specific events and the
EventExecutorService will only support methods which require a client id as a
parameter. The methods without a client id will become deprecated at first.

Thanks for your solutions and ideas. More patches, ideas or support is welcome! 
:-)
That helped a lot!

Original comment by sven.strohschein@googlemail.com on 29 May 2010 at 1:44

[GoogleCodeExporter]

All left problems are now solved in the trunk version. There are now methods to 
add user-specific events and EventFilters directly from the client side. If it 
is necessary to add user-specific events or EventFilters dynamically from the 
server side of a custom service a new "ClientHandler" can now be created. The 
ClientHandler contains the client-id and can be transferred manually to custom 
services or better: The ClientHandler can be added to the custom service with a 
new util-method (RemoteEventServiceFactory#registerClientSpecificHandler(...)).

The documentation is still missing, but multiple sessions are now supported 
(when the feature is configured in the config file).

Original comment by sven.strohschein@googlemail.com on 17 Aug 2010 at 5:39

• Changed state: Fixed
• Added labels: FixedSupportedin1.2

[GoogleCodeExporter]

Thanks Sven!  Someone disabled our client-side event code, so I've only been 
able to do some simple testing using an old version of our code, but it's 
looking good.  

I undid my changes to web.xml, copied conf/eventservice.properties into my 
src/main/resources, changed to "eventservice.connection.id.generator = 
de.novanic.eventservice.service.connection.id.SessionExtendedConnectionIdGenerat
or" and it's all working in my smoke test.

Original comment by sean.flanigan@gmail.com on 6 Sep 2010 at 8:04

[GoogleCodeExporter]

Verified by Sean. Thanks!

Original comment by sven.strohschein@googlemail.com on 7 Sep 2010 at 8:33

• Changed state: Verified

[GoogleCodeExporter]

Just wondering when the 1.2 release may be made available, or if a full patch 
for this issue for 1.1 could be made? We're having the same issue in a FF 
instance of our app and want to resolve it.

Original comment by ericame...@gmail.com on 8 Sep 2010 at 5:55

[GoogleCodeExporter]

When can we expect the official release of 1.2 version?
Thank you guys for the great work and support.

Original comment by yasmeen....@gmail.com on 7 Oct 2010 at 4:23

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Thanks for your great work, Sven. We also encounter this same issue for our 
application working in FF or IE8. How about 1.2? Can we expect it soon :)

Original comment by yijiez...@gmail.com on 24 Nov 2010 at 11:53

[GoogleCodeExporter]

The official release of GWTEventService 1.2 is planned for the second half of 
December, but time could run short. One feature will be extended in the next 
weeks and there is a lot of documentation to be done.

Original comment by sven.strohschein@googlemail.com on 24 Nov 2010 at 10:33

[GoogleCodeExporter]

Hi guys, thank you for the great job you have already done. Could you give an 
update, when you are planning to release gwteventservice 1.2? Thanks in advance.

Original comment by lje...@gmail.com on 4 May 2011 at 12:48