Adding support for recreating channels

[GoogleCodeExporter]

I have done a few tests on how many channels and sockets can be opened at once.

The results are that on the dev server you can create endless channles and for 
each channel you can open endless sockets. And they all work as expected. All 
sockets of one channel receive the messages at the same time.
Also you can close all sockets of a channel and open new ones as you like.

On the production server this is a bit different. Only ONE channel can be 
created and this channel can open just ONE socket.
BUT if the socket is closed you CAN create an other channel and also a new 
socket.
Also if you close the socket you can open a new socket on the same channel.

This patch adresses the issue where you create a new channel if the socket was 
closed.

Original issue reported on code.google.com by buchholz...@googlemail.com on 2 Mar 2011 at 10:58

Attachments:

• ChannelFactory.java.patch

[GoogleCodeExporter]

The patch looks pretty good, I agree it's a bug that you basically have to 
reset everything to get a new channel once the channel is closed.

If createChannel() is called while there is an existing, open channel, should 
it return that channel?  Should it throw an exception?  I think calling 
createChannelImpl() is not the correct behavior, since as you've said, in 
production it won't work.

Original comment by jasonhall@google.com on 3 Mar 2011 at 12:39

[GoogleCodeExporter]

Good points!
I have created two patches both have been tested in dev and production mode.

The first one throws an exception if you are trying to open a second socket. 
Also if a channel was created and the socket was not closed yet it will return 
the current channel instead of creating a new one.

The second one throws an exception if you are trying to open a second socket. 
And it also throws an exception if you are trying to create a new channel while 
a socket is already open.

I prefer the second one because you know what you actually get from the 
ChannelFactory. In the first one you don't know.

Original comment by buchholz...@googlemail.com on 3 Mar 2011 at 2:38

Attachments:

• ChannelNoException.patch
• ChannelException.patch

[GoogleCodeExporter]

This change looks good, I also prefer the second patch.

A couple comments:
- I think I'd prefer to throw an IllegalStateException rather than create our 
own special exception. That way you don't have to catch the 
SocketAlreadyOpenException all the time, when it will only happen hopefully 
very rarely.

- I think there should be different exceptions (or just different messages) for 
attempting to open a second socket on a channel, and for attempting to create a 
channel when one already exists.

Also, as the library starts to do more than just wrap the underlying 
JavaScript, there is more need for tests. I'll file a feature request to add 
basic testing infrastructure, I think it will be helpful to test the behavior 
added in this change as well.

Original comment by jasonhall@google.com on 3 Mar 2011 at 3:34

[GoogleCodeExporter]

Here's an updated patch, let me know what you think.

Original comment by jasonhall@google.com on 3 Mar 2011 at 4:07

Attachments:

• ChannelException2.patch

[GoogleCodeExporter]

There were some errors in there:

First the isOpen property is an instance property and throws 17:39:46.111 
[ERROR] [channeltest] Line 26Instance fields cannot be used in subclasses of 
JavaScriptObject
Patched:  public static boolean isOpen = false;
This works because there can only be one channel.

In the socket.close listener you have 
@com.google.gwt.appengine.channel.client.Channel::isOpen = true;
which should be @com.google.gwt.appengine.channel.client.Channel::isOpen = 
false;

The rest works fine!
Tested also in production mode.

Original comment by buchholz...@googlemail.com on 3 Mar 2011 at 4:54

Attachments:

• Channel.patch

[GoogleCodeExporter]

When is this feature going to be added to the API?

Original comment by JkMcMul...@gmail.com on 6 Jan 2012 at 9:54

[GoogleCodeExporter]

Its been almost a year... any reason this hasnt been merged?

Original comment by bradley....@gmail.com on 20 Feb 2012 at 7:18

[GoogleCodeExporter]

Please merge this patch. We recently noticed the same problem, and had to 
explicitly include the updates.

Original comment by mayumi.l...@gmail.com on 4 Jun 2012 at 4:55

[GoogleCodeExporter]

Can someone please create an updated jar with these patches? :)

Original comment by musclema...@gmail.com on 17 Sep 2012 at 5:12