Closed GoogleCodeExporter closed 8 years ago
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:
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
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
- 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
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
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
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
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
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
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
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
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
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
Verified by Sean. Thanks!
Original comment by sven.strohschein@googlemail.com
on 7 Sep 2010 at 8:33
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
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
[deleted comment]
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
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
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
Original issue reported on code.google.com by
sean.flanigan@gmail.com
on 25 Mar 2010 at 8:03Attachments: