Handling disconnects in Guacamole.Client/WebSocketTunnel

[Or Cohen <or...@strigo.io>]

Hello,

TL;DR: I'm having an issue handling disconnects in
Guacamole.Client/WebSocketTunnel (common-js). I think the tunnel
implementation only goes one way in the state change, i.e. it doesn't
change state to CONNECTING when issuing a connect, causing a state change
to CLOSE to leave the tunnel in a state that eventually forces a complete
replacement of the Guacamole.Client instance.

I'm asking on the dev mailing list because I'd like to contribute a fix,
but it might also be intentional.
Should I open a JIRA issue first?

--

We're using Guacamole in our application (common-js and guacd, the proxy is
our own small implementation around GuacamoleWebSocketTunnelEndpoint), but
I'm having a slight issue handling disconnects.

The purpose is to make the client retry connections infinitely until it
succeeds.
After a successful connection, I forcefully kill the proxy to fake a
disconnect. No matter what I do, I can't get it to "reconnect" and I end
up replacing the entire Guacamole.Client instance.

I think the problem is in the Tunnel implementation for WebSocket (I
suspect it's the same issue for HTTP as well). What I do is listen to state
changes on the tunnel, and when the state changes to "CLOSED", I issue a
connect to the Guacamole.Client (after some delay). I'm not getting a state
change for this second connect call (I believe it's supposed to
be CONNECTING).
Since the state stays CLOSED, the second failure never notifies me and I
can't know of the state change, i.e. that it failed again.

Specifically, this line
http://guacamole.incubator.apache.org/doc/0.9.13-incubating/guacamole-common-js/Tunnel.js.html#line693
is always true after the first state change to CLOSE. There's nothing in
the WebSocket implementation that changes the state to CONNECTING. The
CONNECTING state is just the initial one from the base Tunnel class. This
is not true, for example, for the new Static tunnel, which sets the state
to CONNECTING when connect() is called.

Thanks for the help,
Or.

Re: Handling disconnects in Guacamole.Client/WebSocketTunnel

[Or Cohen <or...@strigo.io>]

On Tue, Aug 1, 2017 at 6:46 PM, Nick Couchman <
nick.couchman@yahoo.com.invalid> wrote:

>
>
>
>
> On Tuesday, August 1, 2017, 11:18:24 AM EDT, Or Cohen <or...@strigo.io>
> wrote:
>
> > Should I open a JIRA issue first?
>
> If your change causes this notification to not be displayed because the
> client continually retries the connection, then the "fix" may not end up
> included in the main Guacamole code.  If it doesn't impact that and have
> other adverse impacts, then it may end up getting merged.
> If it doesn't get merged, you could still maintain the fork of the Apache
> version of the code in your own repo, modify it to your needs, and use it.
> Maintaining the fork would also allow you to continue to merge in changes
> and improvements from the upstream repos as those happen.
>

Of course, I intend to fully try the code to make sure it doesn't break
anything. I have the Guacamole application as a reference implementation.
Knowing (a bit) the Guacamole application, it does reconnect after a
timeout (and offers a manual reconnect) and contiously so.
I see that guacClientManager angular service (and the reconnect button)
actually replaces (disonnects, deletes and reconnects) the entire client.

Now, I suspect that this is intentional and maybe that's why the Tunnel's
state behavior doesn't really affect it.
Perhaps the client can't handle a reconnect properly?

I'll submit a PR. Worst case, it is intentional and won't get accepted :)

Re: Handling disconnects in Guacamole.Client/WebSocketTunnel

[Nick Couchman <ni...@yahoo.com.INVALID>]

On Tuesday, August 1, 2017, 11:18:24 AM EDT, Or Cohen <or...@strigo.io> wrote:

> Should I open a JIRA issue first?

If you have a fix to contribute, yes, the first step is opening a JIRA issue, then forking the github repo, making your change, and opening a pull request where you tag the PR and your commits with the JIRA instance (GUACAMOLE-XXX:).
As far as whether this behavior is intentional or not, Mike or James would probably have to speak to that; however, my question would be this: Are there any consequences to the current Guacamole Client code by making this change?  It looks like you've made a custom application leveraging the guacamole common code - I'd suggest taking the full Guacamole Client code, making your proposed change, and making sure that it still behaves the way it is expected to when the tunnel disconnects.  If I understand the situation you're referring to, in the Guacamole Client code this would mean that a notification box is displayed that the tunnel connection has been lost, and the user is given an option to go home or Reconnect.  If your change causes this notification to not be displayed because the client continually retries the connection, then the "fix" may not end up included in the main Guacamole code.  If it doesn't impact that and have other adverse impacts, then it may end up getting merged.
If it doesn't get merged, you could still maintain the fork of the Apache version of the code in your own repo, modify it to your needs, and use it.  Maintaining the fork would also allow you to continue to merge in changes and improvements from the upstream repos as those happen.
Regards,Nick