FWD » Core Development » User Interface

I am looking into enabling the "port range" idea for launching web clients. Today we support a hard coded client web port or a dynamic mode (port==0) where the port is assigned randomly. Supporting a port range would be an additional mode where the client's web port would be selected from specific pre-set range of ports. This would enable a firewall to be used for better security and should also make it easier to implement a transparent reverse proxy.

To accomplish this, we would need to track the ports from that range that are in use. This means we need to know when the client's embedded web server is shut down. A good place to do this is in ClientCore.start() where we execute this code:

if (!running)
{
            driver.shutdown();
         }

The problem here is that this is called after we call session.terminate() (closing the session with the server) which means we cannot notify the server that the port is free.

I would like to move the embedded web server shutdown to be before the session.terminate(). Logically, I think it is safe to do. My simple test shows this works OK. The only thing I have found is that the client log now has some extra entries:

java.lang.InterruptedException
at java.lang.Object.wait(Native Method)
at java.lang.Object.wait(Object.java:502)
at com.goldencode.p2j.ui.client.driver.web.PushMessagesWorker.waitForMessages(PushMessagesWorker.java:139)
at com.goldencode.p2j.ui.client.driver.web.PushMessagesWorker.run(PushMessagesWorker.java:96)
java.lang.InterruptedException
at java.lang.Object.wait(Native Method)
at java.lang.Object.wait(Object.java:502)
at com.goldencode.p2j.ui.client.driver.web.WebTaskWorker.run(WebTaskWorker.java:62)
Feb 01, 2017 12:43:17 PM org.eclipse.jetty.server.AbstractConnector doStop
INFO: Stopped ServerConnector@27fe3806{SSL,[ssl, http/1.1]}{localhost:0}
Feb 01, 2017 12:43:17 PM org.eclipse.jetty.server.handler.ContextHandler doStop
INFO: Stopped o.e.j.s.h.ContextHandler@4d1b0d2a{/common,jar:file:/home/ges/projects/app/poc/deploy/lib/p2j.jar!/com/goldencode/p2j/ui/client/driver/web/res,UNAVAILABLE}
Feb 01, 2017 12:43:17 PM org.eclipse.jetty.server.handler.ContextHandler doStop
INFO: Stopped o.e.j.s.h.ContextHandler@5c072e3f{/client,jar:file:/home/ges/projects/app/poc/deploy/lib/p2j.jar!/com/goldencode/p2j/ui/client/gui/driver/web/res,UNAVAILABLE}
Feb 01, 2017 12:43:17 PM org.eclipse.jetty.server.handler.ContextHandler doStop
INFO: Stopped o.e.j.s.h.ContextHandler@10a035a0{/,null,UNAVAILABLE}

From Constantin:

What if the client dies unexpectedly and the server is never notified the port is free? This can lead to exhaustion of the avail ports pretty fast...

Why not track at server-side which ports are in-use, and when the client's session terminates (on server-side), add that port back to the pool?

From Constantin:

What if the client dies unexpectedly and the server is never notified the port is free? This can lead to exhaustion of the avail ports pretty fast...

Why not track at server-side which ports are in-use, and when the client's session terminates (on server-side), add that port back to the pool?

That is my plan. The problem with the current order is that the session would go away first, so we would put the port back into the pool before the client's embedded web server has a chance to shutdown. I'm trying to eliminate that race condition by inverting the order of shutdown on the client side.

Constantin wrote:

Ah, OK, then it makes sense.

Sergey wrote:

I think these logs show the correct sequence of shutdown() method invocation:

@Override
public void shutdown() {
// close the emulated windows first
super.shutdown();

// notify the remote client about application exit
websock.quit();

// since the application will exit shutdown the server
shutdownServer();
}

where websock.quit() invokes System.exit(0) on receiving MSG_QUIT from the websocket client and two thead PushMessageWorker and WebTaskWorker are interrupted their WAIT states.

where websock.quit() invokes System.exit(0) on receiving MSG_QUIT

I don't see that WebClientProtocol.quit() calls System.exit(). Where do you see this code?

Sergey wrote:

I didn't want to confuse this shutdown use case. I meant that getting MSG_QUIT from the websocket client invokes that method

public boolean processBinaryMessage(byte[] message, int offset, int length) {

boolean handled = false;

if (length 1 && message[offset] MSG_PAGE_LOADED) {
..............................................

else if (message[offset] MSG_QUIT && length 1) {
// exit the client session
System.exit(0);
}

return handled;
}

It is a good point. I had forgotten we did that. For now I'm going to stop working on this idea.

Sergey wrote:

Another thought is that we don't start watch dog timer here.

// notify the remote client about application exit
websock.quit(); should invoke WatchdogTimer with the server shutdown on receiving MSG_QUIT or by timeout.

// since the application will exit shutdown the server
shutdownServer();

Thus I think the current shutdown logic is incorrect.

Do you agree?

Do you agree?

I think we do need to consider it, but I seem to recall that we deliberately don't use the watchdog in that case. Consider that when the embedded web server is being stopped, it is expected that there will no longer be anyone connecting. Notifying the JS client is a "best efforts" thing. There are many possible race conditions.

As you note: the JS client can trigger a System.exit() while we are trying to close the session with the FWD server. It will need some research and if we went forward with it, much testing.

On further thought, we cannot implement the server-side termination hooks as the only solution for managing the available port list. The issue here is that on CTRL-C, we "restart" the client without exiting. The web server stays running but the connection to the FWD server is terminated and then restarted (running == true in ClientCore.start()). We might be able to use it with a kind of FWD server-side watchdog timer. Since there are so many complications with the session management here, I'm postponing work on this.

Greg, please help to understand the remote case. There are two cases: 1) the web client is spawned locally and 2) the web client is spawned remotely. In the first case if we specify a range of ports, then these ports are allocated for this server host. But in the second case there are remote broker clients that are used to start the web client remotely. These brokers are assigned to a user's name and from the set of assigned brokers the server spawner selects the one that has the minimal workload on the system. It seems that the port range adds complexity to this logic. Are these brokers can be started for different hosts? If there are different hosts, then we should get at least their IPs addresses or their domain names. The reverse proxy should know an exact IP address or a domain name of the hosts on which the web client will be spawned, depending on the private network architecture. The reverse proxy configuration depends on the port range and on the hosts range. In 2683 we develop this mapping

1) https://proxy/gui <-> https://backend:7443/gui
2) https://proxy/7449/index.html <-> https://backend:7449/index.html
3) https://proxy/7449/* <-> https://backend:7449/$1

1) https://proxy/server1/gui <-> https://backend1:7443/gui
2) https://proxy/server1/7449/index.html <-> https://backend1:7449/index.html
3) https://proxy/server1/7449/* <-> https://backend1:7449/$1

Actually, 2683 and 3236 can be developed as a one task due to interdependency of these tasks on the reverse proxy configuration. At least 3236 should be developed first.

The highest priority is getting the local spawning working with port mapping. Please work on that first and we will try to get that into the trunk ASAP so that we can deliver a production reference VM that could be used in a cloud environment like AWS.

As a second phase (in a different task branch) we can work on the remote spawner case.

• Each remote spawner will need its own configuration including port (possibly a range, but also it could be 0 or a fixed port).
• We have a user account affinity issue. If we randomly pick a remote spawner (like today) we must know that the OS user account being launched exists on all of the remote spawners.
• The proxy configuration needs to be enhanced to handle the remote spawning case as you mentioned.

Code Review Task Branch 2683a Revision 11156

Overall the approach is OK. Is it working in your testing?

1. The WebClientRegistrator usage in ClientCore should not be used if the client is not a web client.

2. Change the name of WebClientRegistrator to WebClientRegistrar.

3. The WebClientsManager.getFreeLocalPort() needs to be atomic. Right now it is a race condition. Two requests to that method that are close enough can collide. When it allocates a port, it needs to add that port to the usedLocalPorts before returning so that the port can never be "found" twice.

4. What happens if the ChUI user executes CTRL-C or the GUI user executes ESC? Does the client properly reopen the session using the same port?

5. What happens of the MSG_QUIT comes back from the JS side before the driver.shutdown() has completed? This seems like a race condition where the client JVM may trigger System.exit() before the notification is sent to the FWD server.

6. The current FWD design for session listeners is pretty weird. This is not your fault. Since SessionManager.listen() can only accept a single listener, that listener can now only ever be used for this web client use case. That doesn't make much sense. And if web clients are not used, then we have the listener there when it is not needed. For now, it is OK, but the use of it highlights the problem we already had.

7. I guess that the hard coded port 7449 is just for testing purposes. The allocation needs to be bounded in a range and it must be configurable.

4. What happens if the ChUI user executes CTRL-C or the GUI user executes ESC? Does the client properly reopen the session using the same port?

If the JS web client is not closed or logged out, then the same port is used, otherwise new web client should be allocated if a user tries to logged in. But there is a problem with this requirement now. If a user's session is terminated, then this port can be given to new client that is about to be logged, but this port is still in used by the web client that gets CTRL_C or ESC. Is my view correct?

Does it mean that we should check if the session is terminated, but the web client is still running?

Greg Shah wrote:

Code Review Task Branch 2683a Revision 11156

Overall the approach is OK. Is it working in your testing?

Yes, it works now, but the current implementation is incorrect (has many issues to be resolved).

If a user's session is terminated, then this port can be given to new client that is about to be logged, but this port is still in used by the web client that gets CTRL_C or ESC. Is my view correct?

Yes, that is correct. This is a problem.

Greg, if we have the case when the ports are given and all of them are used by the current clients, then new local users that are in the server's private network can not connect to the server too. Is it a correct requirement?

<node class="container" name="portsRange">
  <node class="string" name="namePrefix">
    <node-attribute name="value" value="client"/>
  </node>
  <node class="integer" name="from">
    <node-attribute name="value" value="7449"/>
  </node>
  <node class="integer" name="to">
    <node-attribute name="value" value="7549"/>
  </node>
</node>

if we have the case when the ports are given and all of them are used by the current clients, then new local users that are in the server's private network can not connect to the server too.

Yes, correct.

Committed revision 11159 added portsRange node under webClient parent node and new business logic to release allocated resources. Working to fix the opened 1) and 5) issues.

Greg, does it make sense to create a configuration that provides a mapping of a user id to its web client port number? In that case each user has its own dedicated port number for the web client. And the web client allocation will be succeeded only if this user will be authenticated. Now the client process is spawned and only then the client is authenticated. Why the system don't authenticate the user before the spawn process is started?

Why the system don't authenticate the user before the spawn process is started?

I understand, because the system is checking its OS credential.

Greg, don't matter my questions. It seems that one user must be able to create several web clients sessions with the server.

You've got it. It is always nice when someone answers their own questions. :)

I encountered the following major security risk as Denial of Service attacks at the application level, not at the tcp/ip level that simply don't give anybody to connect to the server via the web client. The fluid of several simulated login requests to log into the server with a fake user/password pair leads to denial of service since all ports will be concurrently allocated and then will be returned to the pool and then will become again allocated. At the moment the current schema makes this possible.

It seems that it is required to check the provided user/password pair and only then try to allocate the web client. Greg, what do you think? The user/password checking functionality can be similar to the one implemented for GWT Administration client.

This denial of service for non restricted ports is not possible, since the ports is not allocated before the user's OS credentials are verified and they are verified before the web client http server is started. But in the restricted ports case we allocate port number first and then try to spawn the web client.

It seems that Spawner interface should be changed to be able to allocate clients resources if it is required. Planning to test this way.

Sergey Ivanovskiy wrote:

It seems that it is required to check the provided user/password pair and only then try to allocate the web client. Greg, what do you think? The user/password checking functionality can be similar to the one implemented for GWT Administration client.

Agreed. This means the client will have to "up call" to the FWD server to allocate and return the port.

I added new methods to allocate and to release system resources to the Spawn interface. It fixed only a part of that issue, the flow of external requests can still impact on the host that spawns processes. The system creates a one new OS process per an external request. Does it need to implement CAPTCHA for the login screen?

The system creates a one new OS process per an external request. Does it need to implement CAPTCHA for the login screen?

For now, I don't think this is needed.

This problem should only occur with the virtual desktop mode. This is not expected to be used for production. It is useful for testing.

If we get to the point where a customer expects to use virtual desktop mode for production, we will revisit this issue. Please create a new task ("eliminate denial of service in virtual desktop mode") to describe this work.

I do think we should have a configuration option that will disable virtual desktop mode. This way, a production server would be more locked down. Please add this.

I don't understand how to detect if it is a POST request for the virtual desktop mode or the embedded client mode. We have these directory settings for the virtual desktop mode

<node class="container" name="webClient">
  <node class="boolean" name="enabled">
    <node-attribute name="value" value="TRUE"/>
  </node>
  <node class="boolean" name="embedded">
    <node-attribute name="value" value="FALSE"/>
  </node>
.................................................
</node>

If virtual desktop mode is disabled, can't we just refuse to respond to all requests for path /gui?

It seems that if the server refuses GET and POST requests for /gui, then the web client session can be established and the embedded client can't be started.

Then in this mode can you just refuse non-embedded mode sessions in WebHandler?

Greg Shah wrote:

Then in this mode can you just refuse non-embedded mode sessions in WebHandler?

It seems that there are no differences at this level between requests to start embedded or non-embedded clients. Embedded option is used for the JS client only, but the JS client is loaded only after the web client process is started. Please explain more thoroughly what are non-embedded mode sessions.

The embedded mode comes through the WebClientLauncher remote object which has the spawn() entry point. It does not connect through a web POST or GET request.

Thank you, I missed this entry point and didn't implement the port range for clients created by WebClientLauncher. Please help me with examples where this interface is used.

See hotel_gui/embedded/src/com/goldencode/testcases/embedded/CustomP2JClientApp.java. This is the embedded mode client that connects to the FWD server and launches embedded mode web clients.

Thank you, I see it now. Is it required to implement the port range for this type of clients too? Now they are skipped.

Yes, this is actually the most important type of client because most (if not all) customers will be using this approach as their production web GUI.

Ok, understand, I will add the existing functionality for this case too.

Committed revision 11164 added new options to the directory: forwardedHost, forwardedProto and virtalDesktopEnabled. The virtual desktop is disabled by default. These options forwardedHost and forwardedProto override corresponding HTTP headers: "X-Forwarded-Host" and "X-Forwarded-Proto". forwardedHost and forwardedProto are required if there are several proxies in the route to the server. This directory fragment lists all new additional options.

        <node class="container" name="webClient">
..............................................................
          <node class="container" name="portsRange">
            <node class="string" name="namePrefix">
              <node-attribute name="value" value="client"/>
            </node>
            <node class="integer" name="from">
              <node-attribute name="value" value="7449"/>
            </node>
            <node class="integer" name="to">
              <node-attribute name="value" value="7549"/>
            </node>
          </node>
          <node class="string" name="forwardedHost">
            <node-attribute name="value" value="www.goldencode.com"/>
          </node>
          <node class="string" name="forwardedProto">
            <node-attribute name="value" value="https"/>
          </node>
          <node class="boolean" name="virtualDesktopEnabled">
            <node-attribute name="value" value="FALSE"/>
          </node>
..................................

Greg,

5. What happens of the MSG_QUIT comes back from the JS side before the driver.shutdown() has completed? This seems like a race condition where the client JVM may trigger System.exit() before the notification is sent to the FWD server.

It seems there are 2 scenarios depending on how MSG_QUIT is initiated
1) MSG_QUIT is initiated by a user. It happens if a user reloads the current page. Now in this case a user is redirected to the login page.(?)
2) MSG_QUIT is send by the web client. In this case we have the same scenario the JS client gets MSG_QUIT and a user is redirected to the login page.

Please recall what functionality is expected to be if the current page is reloaded by a user. In the current implementation a user is redirected to the login page.
And if the web client is under the reverse proxy and the JS is out of this network, then the reload takes more than a second, on my environment is 2945 ms. But the redirect logic depends on timing now (it is supposed that it takes no more than one second) and in the worse case a user isn't redirected. Please look at this logs that displays the reload use case now. I increased this interval up to 4 seconds.

We can see that index.html , claro.css, dojo.js are loaded from the remote server. Now if a user is not redirected, then the web client is alive, the web socket chanel handles ping and pong messages, but the web client doesn't send the current application windows. It seems that this task is not implemented yet.

GET
https://127.0.0.1/server/client1/index.html [HTTP/1.1 200 OK 46ms]
Connection closed with the returned code 1001 p2j.screen.js:3227:7
GET
https://127.0.0.1/server/client1/dojo-toolkit/dijit/themes/claro/claro.css [HTTP/1.1 200 OK 38ms]
GET
https://127.0.0.1/server/client1/dojo-toolkit/dojo/dojo.js [HTTP/1.1 200 OK 77ms]
GET
https://127.0.0.1/server/client1/common/p2j.js [HTTP/1.1 304 Not Modified 28ms]
GET
https://127.0.0.1/server/client1/common/p2j.logger.js [HTTP/1.1 304 Not Modified 48ms]
GET
https://127.0.0.1/server/client1/common/p2j.sound.js [HTTP/1.1 304 Not Modified 73ms]
GET
https://127.0.0.1/server/client1/client/p2j.strokes.js [HTTP/1.1 304 Not Modified 73ms]
GET
https://127.0.0.1/server/client1/client/p2j.canvas_renderer.js [HTTP/1.1 304 Not Modified 69ms]
GET
https://127.0.0.1/server/client1/client/p2j.virtual_desktop.js [HTTP/1.1 304 Not Modified 89ms]
GET
https://127.0.0.1/server/client1/client/p2j.mouse.js [HTTP/1.1 304 Not Modified 108ms]
GET
https://127.0.0.1/server/client1/client/p2j.screen.js [HTTP/1.1 304 Not Modified 117ms]
GET
https://127.0.0.1/server/client1/common/p2j.socket.js [HTTP/1.1 304 Not Modified 143ms]
GET
https://127.0.0.1/server/client1/common/p2j.keymap.js [HTTP/1.1 304 Not Modified 125ms]
GET
https://127.0.0.1/server/client1/common/p2j.keyboard.js [HTTP/1.1 304 Not Modified 116ms]
GET
https://127.0.0.1/server/client1/client/p2j.clipboard_helpers.js [HTTP/1.1 304 Not Modified 122ms]
GET
https://127.0.0.1/server/client1/common/p2j.clipboard.js [HTTP/1.1 304 Not Modified 145ms]
GET
https://127.0.0.1/server/client1/common/p2j.fonts.js [HTTP/1.1 304 Not Modified 148ms]
GET
https://127.0.0.1/server/client1/common/p2j.remote.js [HTTP/1.1 304 Not Modified 144ms]
GET
https://127.0.0.1/server/client1/ajax [HTTP/1.1 101 Switching Protocols 24ms]
Elapsed: 2945 ms  p2j.socket.js:3177:10
Connection closed with the returned code 1005 p2j.screen.js:3227:7
https://127.0.0.1/gui
GET
https://127.0.0.1/gui

Obviously the time condition in isRequiredToRedirect() is there to distinguish the case when the page loads right after F5 and the case when the reconnection timer fires.

Sergey, as you point out the time condition is unreliable as the time the page loads varies depending on client or server state, network conditions, etc. As such it should be removed and I believe the following steps are the way to go:

1. isRequiredToRedirect() should be removed.
2. In createWebSocket() the onopen handler: when the message to the client web server MSG_PAGE_LOADED fails (timeout, unrecoverable netowrk error, session does not exist (if this can happen), etc.) then the redirect to login page should be performed.
3. MSG_QUIT should be send in onpagehide.

Thank you, it simplifies the current logic. It seems that it should work as we expected.

No, it has an issue with the requirement to reconnect to the web client if the network is temporarily down and then is on again. We can't distinguish the case if it is error due to MSG_PAGE_LOADED fails or connectivity timer fails trying to restore the connection.

Hynek Cihlar wrote:

Obviously the time condition in isRequiredToRedirect() is there to distinguish the case when the page loads right after F5 and the case when the reconnection timer fires.

It seems that it is here to distinguish reload use case from reopen use case, because it checks if the marker object "exitTheApplication" is present, I think that the watchdog timeout of the server can help to distinguish the reopen case from the reload case, because if the JS client will be disconnected from the web client within the time that exceeds the watchdog timeout, then the old web client will be closed. Committed revision 11165 fixed misspelled code that sets incorrectly "watchdogTimeout" value.

Committed revision 11167 prepared this branch 2683a for the review.

Code Review Task Branch 2683a Revision 11167

I have checked in some changes to fix code formatting issues. Overall, I like the result.

1. In ClientCore, please cache the WebClientRegistrar instance obtained on line 263 so that the freeWebClientResources() on line 385 is called on an existing instance instead of obtaining a new one.

2. In WebClientsManager.allocateClient(), is it safe to always hard code the webClientHost to localhost?

3. Please add javadoc for ProcessClientSpawner.releaseClient(), SpawnerImpl.releaseClient(), WebClientSpawner.webClientsManager, WebClientSpawner.requestParameters, WebClientSpawner.TemporaryClientTask.proxyServerParameters.

• have you tested if the FWD client terminates abruptly? In this case, the server-side needs to cleanup the resources on its own. This code in ClientCore is correct:
  

              if (!running)
              {
                 if (referrer != null)
                 {
                    WebClientRegistrar registrar =
                             (WebClientRegistrar) RemoteObject.obtainNetworkInstance(
                                      WebClientRegistrar.class, session);
                    registrar.freeWebClientResources(uuid);
                 }
              }
              session.terminate();
  

  
  as the session is terminated only after the web client resources are freed, but WebClientsManager.terminate should do this, too.

If the web client is failed, then its session is closed, and it seems that all listeners should be notified and terminate() invoked, should not they?

Sergey Ivanovskiy wrote:

If the web client is failed, then its session is closed, and it seems that all listeners should be notified and terminate() invoked, should not they?

Yes, if the FWD session is terminated, then WebClientsManager.terminate is executed. I missed the part where isStillAlive calls freeClient (which releases the resources), but this is done after the timeout (60 seconds), correct? It seems a little to late... why not do this immediately, as the FWD session can't be re-established (as the FWD client is already dead/disconnected/etc and in no way can recover the same context)? Otherwise, the port will be kept as used for the duration of the timeout.

Constantin Asofiei wrote:

Sergey Ivanovskiy wrote:

If the web client is failed, then its session is closed, and it seems that all listeners should be notified and terminate() invoked, should not they?

Yes, if the FWD session is terminated, then WebClientsManager.terminate is executed. I missed the part where isStillAlive calls freeClient (which releases the resources), but this is done after the timeout (60 seconds), correct? It seems a little to late... why not do this immediately, as the FWD session can't be re-established (as the FWD client is already dead/disconnected/etc and in no way can recover the same context)? Otherwise, the port will be kept as used for the duration of the timeout.

I did this because we can't free a given port until its system resources are released. But we didn't know if this session is terminated and the client is not alive or is about to reconnect with new session. For users it means that if the resource pool is exhausted, then a user can get message all resources are used, but after refresh it can get a free connection. It seems that it is a correct behaviour.

Sergey Ivanovskiy wrote:

I did this because we can't free a given port until its system resources are released.

You are referring the client releasing its associated port, correct?

But we didn't know if this session is terminated and the client is not alive or is about to reconnect with new session.

When SessionListener.terminate is called, the FWD client is no longer available/seen by the FWD server - the client can be dead, network connection to the FWD server dropped, etc. For the FWD server, this FWD client can no longer be reconnected, and all its resources need to be cleaned up. But the server side doesn't know if the FWD client process is still alive or not... it sees only that its session was terminated.

For users it means that if the resource pool is exhausted, then a user can get message all resources are used, but after refresh it can get a free connection. It seems that it is a correct behaviour.

What I'm trying to confirm is that the FWD server always frees the resources, for a FWD client which has disconnected from the server. When isStillAlive is called from terminate, at least it should continue to attempt a request until a failure occurs: currently, if the client is still alive (but not connected to the FWD server), and isStillAlive is called from terminate, there is a chance for the request to succeed and the resources will not be released.

So, I'm thinking that isStillAlive should have a boolean parameter which 'forces' the resource cleanup, when called from terminate: it will execute in a loop (with some Thread.sleep code) until the HTTP request fails and resources are cleaned up.

Code Review Task Branch 2683a.

In addition to Greg's and Constantin's finds.

• In WebClientSpawner
  • if (!"/".equals(webClientConfig.getWebRoot())) what if webClientConfig.getWebRoot() returns an empty/blank string or null? should the config value be overriden in this case? also it would be safer to trim the compared string values or to use URI class for the comparison.
  • it would be safer to do spawner.releaseClient(uuid); in a finally block. this would cover the expected case of uri == null as well as any unexpected error conditions
  • using URI(String scheme, String host, String path, String fragment) instead of URI.create(forwardedProto + "://" + forwardedHost + forwardedPath + "/") would improve robustness as the leading slash in forwardedPath or the ending slash in forwardedHost would not have to be expected.
• portsResticted => portsRestricted
• WebClientsManager: // TODO Auto-generated catch block
• Below the port should be given out when isStillAlive() returns false. Also this may be a performance bottleneck once the number of clients increases. I think it would help if the network check would be performed only when no free port is found in usedLocalPorts.
  

        while (freePort <= to)
  +      {
  +         WebClientConfig config = usedLocalPorts.get(freePort);
  +         if (config == null)
  +         {
  +            return freePort;
  +         }
  +         else
  +         {
  +            isStillAlive(config);
  +         }
  +         
  +         freePort++;
  +      }
  

• sendForbiddenIfVirtualDesktopDisabled is missing @return javadoc
• a hardcoded https: String forwardedProto = "https"; //base.getHeader(HttpHeader.X_FORWARDED_PROTO.asString());
• DojoToolkitHandler should be changed to take into account a non-emoty webRoot (and probably all the other resource handlers (js, mp3, wav, etc) too?)

Constantin Asofiei wrote:

What I'm trying to confirm is that the FWD server always frees the resources, for a FWD client which has disconnected from the server. When isStillAlive is called from terminate, at least it should continue to attempt a request until a failure occurs: currently, if the client is still alive (but not connected to the FWD server), and isStillAlive is called from terminate, there is a chance for the request to succeed and the resources will not be released.

All resources will be released on new requests to log in via the web client.

So, I'm thinking that isStillAlive should have a boolean parameter which 'forces' the resource cleanup, when called from terminate: it will execute in a loop (with some Thread.sleep code) until the HTTP request fails and resources are cleaned up.

I missed this point because I think that according to ClientCore the session can be terminated, but the web client as a process can be alive and then new session can be initiated with the same parameters for the embedded web server. What is the purpose of running flag? If the web client as a instance of ClientCore can't create new session, then we can implement it. Please explain more thoroughly why the web client can't reestablish new session with the server.

Hynek Cihlar wrote:

Code Review Task Branch 2683a.

In addition to Greg's and Constantin's finds.

• In WebClientSpawner
  • if (!"/".equals(webClientConfig.getWebRoot())) what if webClientConfig.getWebRoot() returns an empty/blank string or null? should the config value be overriden in this case? also it would be safer to trim the compared string values or to use URI class for the comparison.
  • it would be safer to do spawner.releaseClient(uuid); in a finally block. this would cover the expected case of uri == null as well as any unexpected error conditions
  • using URI(String scheme, String host, String path, String fragment) instead of URI.create(forwardedProto + "://" + forwardedHost + forwardedPath + "/") would improve robustness as the leading slash in forwardedPath or the ending slash in forwardedHost would not have to be expected.
• portsResticted => portsRestricted
• WebClientsManager: // TODO Auto-generated catch block
• Below the port should be given out when isStillAlive() returns false. Also this may be a performance bottleneck once the number of clients increases. I think it would help if the network check would be performed only when no free port is found in usedLocalPorts.
  [...]

• sendForbiddenIfVirtualDesktopDisabled is missing @return javadoc
• a hardcoded https: String forwardedProto = "https"; //base.getHeader(HttpHeader.X_FORWARDED_PROTO.asString());

Planning to fix them.

• DojoToolkitHandler should be changed to take into account a non-emoty webRoot (and probably all the other resource handlers (js, mp3, wav, etc) too?)

It seems that DojoToolkitHandler should not be changed, because it is not a web virtual directory for the embedded server, but a user sees it like a virtual directory. webRoot is a mapping for the reverse proxy.

It seems that DojoToolkitHandler should not be changed, because it is not a web virtual directory for the embedded server, but a user sees it like a virtual directory. webRoot is a mapping for the reverse proxy. For users in the same network it is a root "/", but for others it is a none empty string like /server/client1/

Sergey Ivanovskiy wrote:

It seems that DojoToolkitHandler should not be changed, because it is not a web virtual directory for the embedded server, but a user sees it like a virtual directory. webRoot is a mapping for the reverse proxy. For users in the same network it is a root "/", but for others it is a none empty string like /server/client1/

Good, you would find this one right away anyway.

Hynek Cihlar wrote:

Code Review Task Branch 2683a.

• In WebClientSpawner
  • if (!"/".equals(webClientConfig.getWebRoot())) what if webClientConfig.getWebRoot() returns an empty/blank string or null? should the config value be overriden in this case? also it would be safer to trim the compared string values or to use URI class for the comparison.
  • it would be safer to do spawner.releaseClient(uuid); in a finally block. this would cover the expected case of uri == null as well as any unexpected error conditions
  • using URI(String scheme, String host, String path, String fragment) instead of URI.create(forwardedProto + "://" + forwardedHost + forwardedPath + "/") would improve robustness as the leading slash in forwardedPath or the ending slash in forwardedHost would not have to be expected.

At the moment I don't have another design construction, now webClientConfig.getWebRoot() is always not null and can be "/" or the special mapping.

Hynek Cihlar wrote:

Code Review Task Branch 2683a.

• In WebClientSpawner
• Below the port should be given out when isStillAlive() returns false. Also this may be a performance bottleneck once the number of clients increases. I think it would help if the network check would be performed only when no free port is found in usedLocalPorts.

while (freePort <= to)
+      {
+         WebClientConfig config = usedLocalPorts.get(freePort);
+         if (config == null)
+         {
+            return freePort;
+         }
+         else
+         {
+            isStillAlive(config);
+         }
+         
+         freePort++;
+      }

Please explain what is the bottleneck?

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Code Review Task Branch 2683a.

• In WebClientSpawner
• Below the port should be given out when isStillAlive() returns false. Also this may be a performance bottleneck once the number of clients increases. I think it would help if the network check would be performed only when no free port is found in usedLocalPorts.

[...]

Please explain what is the bottleneck?

The bottleneck is the isStillAlive() which does a network IO to find out whether a client is still alive.

I agree if you mean that https involves heavy calculations. Committed rev. 11169 fixed all comments that I could do.

Sergey Ivanovskiy wrote:

I agree if you mean that https involves heavy calculations.

Even without https, one check may cost you several milliseconds (and depending on the network conditions up to tens of milliseconds).

If I understand correctly, the check only occurs if we can't allocate a port. In the common case (there is a free port available), this doesn't gt executed.

Perhaps it is OK in this scenario?

Greg Shah wrote:

If I understand correctly, the check only occurs if we can't allocate a port.

Unless I am reading the code wrong, the method goes through all active/nonactive clients and performs the net IO, until it hits a free port. Also the method is called on client session start and on client session termination.

Sergey Ivanovskiy wrote:

So, I'm thinking that isStillAlive should have a boolean parameter which 'forces' the resource cleanup, when called from terminate: it will execute in a loop (with some Thread.sleep code) until the HTTP request fails and resources are cleaned up.

I missed this point because I think that according to ClientCore the session can be terminated, but the web client as a process can be alive and then new session can be initiated with the same parameters for the embedded web server. What is the purpose of running flag?

When false, the running flag determines if the FWD client is terminating.

If the web client as a instance of ClientCore can't create new session, then we can implement it. Please explain more thoroughly why the web client can't reestablish new session with the server.

When true, running flag indicates the client must restart, but the FWD session will not be re-used - a new one will be created. Also, in this case, registrar.freeWebClientResources(uuid); will not be called, but a new uuid will be created... see line 266, which executes registrar.registerWebClientSession(uuid, session.getNodeAddress()); on each iteration of this running loop. I assume that the web driver will still re-use the assigned port (during spawn), but it will not be able to clean it, as the UUID will no longer match.

1. FWD client is executing just before ClientCore.start:366:
   

            if (!running) // client is here
            {
               driver.shutdown();
            }
   

2. assume for some reason the client-server FWD connection is broken; now, FWD server will receive a terminate event and call isStillAlive, but it can see that connectTest is still responsive, thus it doesn't clean.
3. the FWD client continues executing, but it can't call the server side to cleanup.

The two cases above are both possible scenarios where the port can be 'leaked'. Eventually, it will be determined as 'not in use' by getFreeLocalPort, so at least we have this.

Constantin Asofiei wrote:

When true, running flag indicates the client must restart, but the FWD session will not be re-used - a new one will be created. Also, in this case, registrar.freeWebClientResources(uuid); will not be called, but a new uuid will be created... see line 266, which executes registrar.registerWebClientSession(uuid, session.getNodeAddress()); on each iteration of this running loop. I assume that the web driver will still re-use the assigned port (during spawn), but it will not be able to clean it, as the UUID will no longer match.

Sorry, the UUID is not recreated, so this case looks OK.

Hynek Cihlar wrote:

Greg Shah wrote:

If I understand correctly, the check only occurs if we can't allocate a port.

Unless I am reading the code wrong, the method goes through all active/nonactive clients and performs the net IO, until it hits a free port. Also the method is called on client session start and on client session termination.

Hmm. You're right.

Sergey: it would be better to find a free port without doing the isStillAlive() check and only use this if all ports are unavailable.

Constantin Asofiei wrote:

Beside the above, what I have in mind is a possible race condition; consider these execution points, concurrently:

1. FWD client is executing just before ClientCore.start:366:
   [...]
2. assume for some reason the client-server FWD connection is broken; now, FWD server will receive a terminate event and call isStillAlive, but it can see that connectTest is still responsive, thus it doesn't clean.
3. the FWD client continues executing, but it can't call the server side to cleanup.

The two cases above are both possible scenarios where the port can be 'leaked'. Eventually, it will be determined as 'not in use' by getFreeLocalPort, so at least we have this.

Constantin, does it mean that this problem of leaking resources described above exists in the trunc version? The web client is running, using the port, but the server and the client communication loses forever.

Greg Shah wrote:

Hynek Cihlar wrote:

Greg Shah wrote:

If I understand correctly, the check only occurs if we can't allocate a port.

Unless I am reading the code wrong, the method goes through all active/nonactive clients and performs the net IO, until it hits a free port. Also the method is called on client session start and on client session termination.

Hmm. You're right.

Sergey: it would be better to find a free port without doing the isStillAlive() check and only use this if all ports are unavailable.

Ok, this method isStillAlive() works asynchronously, it only loads the server for a while.

Sergey Ivanovskiy wrote:

Greg Shah wrote:

Hynek Cihlar wrote:

Greg Shah wrote:

If I understand correctly, the check only occurs if we can't allocate a port.

Unless I am reading the code wrong, the method goes through all active/nonactive clients and performs the net IO, until it hits a free port. Also the method is called on client session start and on client session termination.

Hmm. You're right.

Sergey: it would be better to find a free port without doing the isStillAlive() check and only use this if all ports are unavailable.

Ok, this method isStillAlive() works asynchronously, it only loads the server for a while.

Still, the extra load can be easily avoided by scanning the usedLocalPorts map and only do the IO when no free port is found.

Since the method is async you need a join point that would be executed when the net IO finishes. An example: the port range is 2, the usedLocalPorts is full, first entry points to a dead client, second to the live. In this case the algorithm should return the port of the dead client, but it doesn't.

Hynek Cihlar wrote:

Still, the extra load can be easily avoided by scanning the usedLocalPorts map and only do the IO when no free port is found.

Since the method is async you need a join point that would be executed when the net IO finishes. An example: the port range is 2, the usedLocalPorts is full, first entry points to a dead client, second to the live. In this case the algorithm should return the port of the dead client, but it doesn't.

You proposed another algorithm, but you reviewed the different method, it works as expected. Please approve that I should implement your proposals.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Still, the extra load can be easily avoided by scanning the usedLocalPorts map and only do the IO when no free port is found.

Since the method is async you need a join point that would be executed when the net IO finishes. An example: the port range is 2, the usedLocalPorts is full, first entry points to a dead client, second to the live. In this case the algorithm should return the port of the dead client, but it doesn't.

You proposed another algorithm, but you reviewed the different method, it works as expected. Please approve that I should implement your proposals.

What do you mean by different method? Below is the diff I see between the branch and trunk. Assuming the diff shows the most recent version the method will return -1 when the configured port range is 2 and usedLocalPorts is filled with one dead client and one alive client, which is clearly not correct.

+   private int getFreeLocalPort()
+   {
+      if (!portsRestricted)
+      {
+         return 0;
+      }
+
+      int freePort = from;
+      
+      while (freePort <= to)
+      {
+         WebClientConfig config = usedLocalPorts.get(freePort);
+         if (config == null)
+         {
+            return freePort;
+         }
+         else
+         {
+            isStillAlive(config);
+         }
+         
+         freePort++;
+      }
+      
+      return -1;
+   }

Yes, it is expected behaviour, but it is only for the case if the almost all resources are used (#range -1), correct? Please consider the case when all resources are exhausted, then new requests can also load the system and users will wait for negative responses.

Sergey Ivanovskiy wrote:

Constantin, does it mean that this problem of leaking resources described above exists in the trunc version? The web client is running, using the port, but the server and the client communication loses forever.

In trunk, we don't have explicit web port management: is either set static (and only one client available at a time) or is set dynamic (and the OS takes care of assigning the port). So, the limit would be reached if no free socket is available at the OS level.

In our case, where we want to manage the ports explicitly (and the range I assume will be limited), we need to make sure we clean up properly...

1. WebClientConfig has an additional counter (synchronized access), which gets incremented each time registerWebClientSession is called.
2. in WebClientsManager.terminate, pass the value of this counter to isStillAlive.
3. in isStillAlive, when called from terminate, there is a loop which:
• if the HTTP request returns as failed, then we know the port is free and just free the resources
• in the for loop, if the counter for the WebClientConfig instance is not the same as the counter received as argument (thus gets modified), then we know the config is being re-used so we can exit the loop safely
• otherwise, if the counter doesn't modify in a certain number of iterations and in all these iterations the HTTP request is passed, log something that the client is 'runaway'. We can't actually free the port, as is in use at the OS level...
• also, as the newRequest call is async, you will need some synchronization to make it sync - use some latch and block on it until the request is processed, and the result is saved.

Sergey Ivanovskiy wrote:

Yes, it is expected behaviour, but it is only for the case if the almost all resources are used (#range -1), correct?

Are you saying that it is an expected bahavior to deny a user access when a port is available to be used but blocked by a dead client entry? If so then please ignore my comments.

Please consider the case when all resources are exhausted, then new requests can also load the system and users will wait for negative responses.

Which is the case with the current implementation, too - when all ports are taken and alive, the method will check all the active clients for every new client access request.

Constantin Asofiei wrote:

Sergey Ivanovskiy wrote:

Constantin, does it mean that this problem of leaking resources described above exists in the trunc version? The web client is running, using the port, but the server and the client communication loses forever.

In trunk, we don't have explicit web port management: is either set static (and only one client available at a time) or is set dynamic (and the OS takes care of assigning the port). So, the limit would be reached if no free socket is available at the OS level.

You considered the case of leaking resources in the general case, the port management that is under development and review doesn't invluence this case. If the system is already has possibility of leaking resources, then how to find ways to manage the web clients?

In our case, where we want to manage the ports explicitly (and the range I assume will be limited), we need to make sure we clean up properly...

I understand what is required.

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Yes, it is expected behaviour, but it is only for the case if the almost all resources are used (#range -1), correct?

Are you saying that it is an expected bahavior to deny a user access when a port is available to be used but blocked by a dead client entry? If so then please ignore my comments.

Please consider the case when all resources are exhausted, then new requests can also load the system and users will wait for negative responses.

Which is the case with the current implementation, too - when all ports are taken and alive, the method will check all the active clients for every new client access request.

I will try to fix it.

Sergey Ivanovskiy wrote:

Which is the case with the current implementation, too - when all ports are taken and alive, the method will check all the active clients for every new client access request.

I will try to fix it.

Sergey, this should be an easy fix. Just scan the usedLocalPorts map first for free ports, when you find one, return it. When you find no free port, issue those asynchronous requests and wait for the first that indicates a dead client. But I would suggest to first work in those inputs from Constantin. Hope this helps.

Ok, thank you, we can try blocking API for this case.

Constantin, Hynek as I understand correctly there is a case when the web client lives forever and can't reconnect to the server again, and the server loses its connection with its web client forever. For this case this web client port can't be used and we have a leak. It doesn't depend on how we manage web clients, correct?

Sergey Ivanovskiy wrote:

Constantin, Hynek as I understand correctly there is a case when the web client lives forever and can't reconnect to the server again, and the server loses its connection with its web client forever. For this case this web client port can't be used and we have a leak. It doesn't depend on how we manage web clients, correct?

Yes, in this case we can't free it as is still in use at the OS level. Something else went wrong and manual intervention (i.e. kill the process) is required.

Sergey Ivanovskiy wrote:

You considered the case of leaking resources in the general case, the port management that is under development and review doesn't invluence this case. If the system is already has possibility of leaking resources, then how to find ways to manage the web clients?

With your changes for virtual desktop mode, a web port is 'in use' only if spawn (and OS-level authentication) passes, correct? So we can't have a leak, just (as you mentioned) a denial of service possibility (and you mentioned this already in a previous comment).

With embedded mode, (for example, how embedded Hotel GUI is built), the FWD iframe will be 'launched' only when authentication data is submitted (i.e. user clicks 'Login'); and if authentication fails, then the FWD client is terminated (and port freed). In this case too, only a DoS is possible.

At this time, and with your changes, I don't think a leak is possible where the FWD web client gets started and web port/connection established without passing the authentication phase.

Committed revision 11170 fixed getFreeLocalPort, now exploring "session counter" from the note 81.

Please review committed revision 11171 that added "session counter" from the note 81.

Code Review Task Branch 2683a Revision 11171

1. The WebClientManager.try{Acquire|Release}Port() methods have a common sleeping loop that probably can be abstracted into a separate method.

2. I think that #3236-85 is not yet addressed. What is your plan there?

3. What notes still need to be addressed?

4. I have checked in some changes to fix code formatting problems. Please be more careful with the code.

5. WebClientConfig.sessionsCounter and its getter/setter methods need javadoc.

6. In WebClientManager please fix the imports to use *.

Constantin: do the changes suitably address your concerns?

Also: perhaps we need to add some comments to make sure that your "leaking resources" concerns are clear to future readers of the code. I don't want people changing the code later and not realizing that such changes could cause this condition.

Greg Shah wrote:

2. I think that #3236-85 is not yet addressed. What is your plan there?

I think it is addressed in the current top branch revision. The code now checks the map of used client ports and only performs net IO when no free one is found.

Sergey, just one thing about the implementation. You should not ignore the InterruptedException in tryAcquirePort(), instead do something like this:

         try
         {
            synchronizer.await(timeout, TimeUnit.MILLISECONDS);
         }
         catch (InterruptedException e)
         {
            Thread.currentThread().interrupt(); // turn the flag back on so that the other call stack frames have a chance to handle the case
            throw new RuntimeException(e);            
         }

         return availablePort.get();

And you no longer need the outer while loop.

I noticed you use this pattern on other place(s), too. Be sure to fix those, too.

Greg Shah wrote:

Constantin: do the changes suitably address your concerns?

Yes.

Also: perhaps we need to add some comments to make sure that your "leaking resources" concerns are clear to future readers of the code. I don't want people changing the code later and not realizing that such changes could cause this condition.

• in the javadoc for tryReleasePort add comments stating that this will try to free the port for a terminated session, but it might not free it if is still in use (client was restarted) or if the client is 'runaway' and the port is open at the OS level, without re-using it.
• in tryReleasePort, if the loop ends with the session counter same as expectedCount, log a message stating that the port couldn't be freed because is still open at OS level and the client is not re-using it.

Committed revision 11173 fixed all found issues, but now I am investigating the case when all two ports are used and the third user gets OutOfResources, and then one of the running client is terminated, so the resource should be free, but the code doesn't work as expected. Planning to fix it today.

Sergey: Please finish item 6 in #3236-92 and handle #3236-95. These should be trivial.

I do need you utility program for creating the reverse proxy configuration. But that tool can be added safely after testing, assuming it doesn't get used at conversion/runtime.

As soon as you have the recently found bug (#3236-96) fixed, is everything else done which is needed for testing?

Greg Shah wrote:

Sergey: Please finish item 6 in #3236-92 and handle #3236-95. These should be trivial.

Committed revision 11173 fixed these trivial issues.

I do need you utility program for creating the reverse proxy configuration. But that tool can be added safely after testing, assuming it doesn't get used at conversion/runtime.

It is not ready yet but it is not difficult, as an example I am using SSLCertGenUtil

As soon as you have the recently found bug (#3236-96) fixed, is everything else done which is needed for testing?

Apache server should be installed and these two modules: mod_proxy and mod_proxy_wstunnel should be enabled. Then it is required to setup Apache configuration to use https and to use the reverse proxy configuration from https://proj.goldencode.com/issues/2683#note-54.

I am using the default /etc/apache2/sites-available/default-ssl.conf.

<IfModule mod_ssl.c>
<VirtualHost _default_:443>
....................
# the reverse proxy settings
#place here configuration of the reverse proxy
</VirtualHost>
</IfModule>

sudo a2enmod proxy
sudo a2enmod proxy_http
sudo a2enmod proxy_ajp
sudo a2enmod rewrite
sudo a2enmod deflate
sudo a2enmod headers
sudo a2enmod proxy_balancer
sudo a2enmod proxy_connect
sudo a2enmod proxy_html
sudo a2enmod proxy_html
sudo a2enmod proxy_wstunnel

Committed revision 11173 fixed these trivial issues.

I don't see any new javadoc for Constantin's request in note 95:

in the javadoc for tryReleasePort add comments stating that this will try to free the port for a terminated session, but it might not free it if is still in use (client was restarted) or if the client is 'runaway' and the port is open at the OS level, without re-using it.
in tryReleasePort, if the loop ends with the session counter same as expectedCount, log a message stating that the port couldn't be freed because is still open at OS level and the client is not re-using it.

is everything else done which is needed for testing?

I do need the full details on setup/config for the reverse proxy. But in this question, I was asking if you can get the 2683a into regression testing today (ChUI stuff on devsrv01 and also manual GUI testing). I need this to be in the trunk ASAP.

Greg Shah wrote:

Committed revision 11173 fixed these trivial issues.

I don't see any new javadoc for Constantin's request in note 95:

in the javadoc for tryReleasePort add comments stating that this will try to free the port for a terminated session, but it might not free it if is still in use (client was restarted) or if the client is 'runaway' and the port is open at the OS level, without re-using it.
in tryReleasePort, if the loop ends with the session counter same as expectedCount, log a message stating that the port couldn't be freed because is still open at OS level and the client is not re-using it.

is everything else done which is needed for testing?

I do need the full details on setup/config for the reverse proxy. But in this question, I was asking if you can get the 2683a into regression testing today (ChUI stuff on devsrv01 and also manual GUI testing). I need this to be in the trunk ASAP.

They are present. Please look at these changes

    /**
-    * Try to release port acquired by the target web client.
+    * Try to release port acquired by the target web client on triggered terminated session events,
+    * but it might not free it if is still in use (client was restarted) or if the client is
+    * 'runaway' and the port is open at the OS level, without re-using it.
     * 
     * @param    config

and

+               if (synchronizer.await(TIMEOUT, TimeUnit.MILLISECONDS))
+               {
+                  return;
+               }
+            }
+         }
+         catch(InterruptedException e)
+         {
+            Thread.currentThread().interrupt();
+            throw new RuntimeException(e);
+         }
+         if (config.getSessionsCounter().compareAndSet(expectedCount, expectedCount))
+         {
+            System.err.println("The port '" + config.getPort() + "' couldn't be freed because" +
+                     "is still open at OS level and the client uuid='" + config.getUuid() +
+                     "' is not re-using it.");
          }
       });

Sorry, I didn't look carefully enough.

What about the readiness for regression/GUI testing?

Sergey Ivanovskiy wrote:

They are present. Please look at these changes
[...]

Please use a logger and not System.err.

The manual GUI and regression testing are in process now, but the current code has some unknown issue and it should be fixed. The proposed schema doesn't work as expected and now I am investigating the reason.

The reverse proxy settings are in note 99. The Apache was setup 4 months ago and I can miss something related to setup but the configuration is clean.

The considered issue is that if all ports are used by the clients and new one user is trying to get a client, then it acquires WebClientsManager and releases it only after TIMEOUT period.

Sergey Ivanovskiy wrote:

The considered issue is that if all ports are used by the clients and new one user is trying to get a client, then it acquires WebClientsManager and releases it only after TIMEOUT period.

Do you count down or interrupt the synchronizer in tryAcquirePort() after all ports are checked? If I remember correctly the synchronizer is interrupted only when the client check fails.

Yes, but it is required to wait until requests timeout is passed. Committed revision 11175 fixed this strict synchronization on WebClientsManager and added logger. This version is ready ffor regression testing. Now planning to do manual GUI tests.

Sergey Ivanovskiy wrote:

Yes, but it is required to wait until requests timeout is passed. Committed revision 11175 fixed this strict synchronization on WebClientsManager and added logger. This version is ready ffor regression testing. Now planning to do manual GUI tests.

In tryAcquirePort() you call synchronizer.countDown() only when client check fails. Thus when all the clients are alive and all ports are taken, the method will sit and wait for the duration of TIMEOUT milliseconds before it returns -1. Which seems like the behavior you mention in note 105. So again, you should do synchronizer.countDown() when all the clients have been tested!

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Yes, but it is required to wait until requests timeout is passed. Committed revision 11175 fixed this strict synchronization on WebClientsManager and added logger. This version is ready ffor regression testing. Now planning to do manual GUI tests.

In tryAcquirePort() you call synchronizer.countDown() only when client check fails. Thus when all the clients are alive and all ports are taken, the method will sit and wait for the duration of TIMEOUT milliseconds before it returns -1. Which seems like the behavior you mention in note 105. So again, you should do synchronizer.countDown() when all the clients have been tested!

You are correct, but it is not a issue. I will do it later.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Yes, but it is required to wait until requests timeout is passed. Committed revision 11175 fixed this strict synchronization on WebClientsManager and added logger. This version is ready ffor regression testing. Now planning to do manual GUI tests.

In tryAcquirePort() you call synchronizer.countDown() only when client check fails. Thus when all the clients are alive and all ports are taken, the method will sit and wait for the duration of TIMEOUT milliseconds before it returns -1. Which seems like the behavior you mention in note 105. So again, you should do synchronizer.countDown() when all the clients have been tested!

You are correct, but it is not a issue.

I thought it was an issue according to your note 105: "The considered issue is that if all ports are used by the clients and new one user is trying to get a client, then it acquires WebClientsManager and releases it only after TIMEOUT period."

You are right, it is an issue.

I have problems with hotel_gui, and investigating them now.

Sergey Ivanovskiy wrote:

I have problems with hotel_gui, and investigating them now.

It seems that the spawned process is failed silently for hotel_gui and a user is redirected to the web client and all resources are loaded, but the web socket is failed.
But with the P2J trunc it starts the web client successfully. so I missed something that should be easy. How to log the spawned process? The part with returned uri works properly, but then the JS client can't connect its web client and logging processes displays that it was failed silently.

Sergey Ivanovskiy wrote:

Sergey Ivanovskiy wrote:

I have problems with hotel_gui, and investigating them now.

It seems that the spawned process is failed silently for hotel_gui and a user is redirected to the web client and all resources are loaded, but the web socket is failed.
But with the P2J trunc it starts the web client successfully. so I missed something that should be easy. How to log the spawned process? The part with returned uri works properly, but then the JS client can't connect its web client and logging processes displays that it was failed silently.

Check the java client log in deploy/client.

Thank you, I see here only logs for P2J trunc's web client, so the embedded server was not started at all and the spawned process is failed before that moment.

Sergey Ivanovskiy wrote:

Thank you, I see here only logs for P2J trunc's web client, so the embedded server was not started at all and the spawned process is failed before that moment.

Did you do gradlew all -Dpost.build=yes -Dspawn.install.folder=/opt/spawner on your branch?

Thank you :), I rerun with 2683a and found that client logs are rewritten.

java.lang.InterruptedException: sleep interrupted
    at java.lang.Thread.sleep(Native Method)
    at com.goldencode.p2j.ui.client.driver.web.WatchdogTimer.run(WatchdogTimer.java:128)
java.lang.RuntimeException: Unresolvable remote export public abstract void com.goldencode.p2j.main.WebClientRegistrar.registerWebClientSession(java.lang.String,int).
    at com.goldencode.p2j.net.RemoteObject$RemoteAccess.obtainRoutingKey(RemoteObject.java:1580)
    at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1464)
    at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
    at com.sun.proxy.$Proxy8.registerWebClientSession(Unknown Source)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:266)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:158)
    at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:250)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
    at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:144)
    at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:313)
Caused by: java.lang.RuntimeException: No export or no access to com.goldencode.p2j.main.WebClientRegistrar:public abstract void com.goldencode.p2j.main.WebClientRegistrar.registerWebClientSession(java.lang.String,int)
    at com.goldencode.p2j.net.HighLevelObject.getKey(HighLevelObject.java:169)
    at com.goldencode.p2j.net.RemoteObject$RemoteAccess.obtainRoutingKey(RemoteObject.java:1552)
    ... 9 more
java.lang.InterruptedException
    at java.lang.Object.wait(Native Method)
    at java.lang.Object.wait(Object.java:502)
    at com.goldencode.p2j.ui.client.driver.web.PushMessagesWorker.waitForMessages(PushMessagesWorker.java:187)
    at com.goldencode.p2j.ui.client.driver.web.PushMessagesWorker.run(PushMessagesWorker.java:144)
java.lang.InterruptedException
    at java.lang.Object.wait(Native Method)
    at java.lang.Object.wait(Object.java:502)
    at com.goldencode.p2j.ui.client.driver.web.WebTaskWorker.run(WebTaskWorker.java:110)

This is an ACL issue. In /security/acl/net/ you need to add a section for WebClientRegistrar to allow access to the exported API. You can pattern it after the section on WebClientLauncher.

Make sure you check in your minimum needed changes to the directory template (for both Hotel ChUI and Hotel GUI) so that we can all use this new feature.

It works now, thank you, I just copied WebClientLauncher section and replaced it with WebClientRegistrar. Are these settings correctly added?

          <node class="container" name="002275">
            <node class="resource" name="resource-instance">
              <node-attribute name="reftype" value="TRUE"/>
              <node-attribute name="reference" value="com.goldencode.p2j.main.WebClientLauncher"/>
            </node>
            <node class="netRights" name="rights">
              <node-attribute name="permissions" value="'0101'B"/>
            </node>
            <node class="strings" name="subjects">
              <node-attribute name="values" value="all_others"/>
            </node>
          </node>
          <node class="container" name="002276">
            <node class="resource" name="resource-instance">
              <node-attribute name="reftype" value="TRUE"/>
              <node-attribute name="reference" value="com.goldencode.p2j.main.WebClientRegistrar"/>
            </node>
            <node class="netRights" name="rights">
              <node-attribute name="permissions" value="'0101'B"/>
            </node>
            <node class="strings" name="subjects">
              <node-attribute name="values" value="all_others"/>
            </node>
          </node>

Yes.

I have the following issue with the embedded client, probably due to certificate issue.
If the port settings in the standard hotel_gui directory.xml for the web client has a port=7449 for which the web client certificate was accepted by the browser, then
the embedded hotel works properly, but if the port number is zero, then the embedded client is not loaded by iframe. The web client is started successfully, there are no errors in its log, but the server log has this error

Jun 30, 2017 8:09:30 AM Protocol.Reader.run() 
WARNING: {Reader} failure in reading loop
javax.net.ssl.SSLProtocolException: Data received in non-data state: 6
    at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:1109)
    at sun.security.ssl.SSLSocketImpl.readDataRecord(SSLSocketImpl.java:930)
    at sun.security.ssl.AppInputStream.read(AppInputStream.java:105)
    at sun.security.ssl.AppInputStream.read(AppInputStream.java:71)
    at java.io.ObjectInputStream$PeekInputStream.peek(ObjectInputStream.java:2598)
    at java.io.ObjectInputStream$BlockDataInputStream.peek(ObjectInputStream.java:2905)
    at java.io.ObjectInputStream$BlockDataInputStream.peekByte(ObjectInputStream.java:2915)
    at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1502)
    at java.io.ObjectInputStream.readObject(ObjectInputStream.java:422)
    at com.goldencode.p2j.net.Protocol$Reader.run(Protocol.java:416)
    at java.lang.Thread.run(Thread.java:748)

    <node class="container" name="server">
      <node class="container" name="default">
        <node class="container" name="webClient">
          <node class="boolean" name="embedded">
            <node-attribute name="value" value="FALSE"/>
          </node>
          <node class="boolean" name="enabled">
            <node-attribute name="value" value="TRUE"/>
          </node>
          <node class="string" name="host">
            <node-attribute name="value" value="localhost"/>
          </node>
          <node class="integer" name="port">
            <node-attribute name="value" value="0"/>
          </node>
          <node class="integer" name="maxBinaryMessage">
            <node-attribute name="value" value="32894"/>
          </node>
          <node class="integer" name="maxIdleTime">
            <node-attribute name="value" value="90000"/>
          </node>
          <node class="integer" name="watchdogTimeout">
            <node-attribute name="value" value="120000"/>
          </node>

    <node class="container" name="server">
      <node class="container" name="default">
        <node class="container" name="webClient">
          <node class="boolean" name="embedded">
            <node-attribute name="value" value="FALSE"/>
          </node>
          <node class="boolean" name="enabled">
            <node-attribute name="value" value="TRUE"/>
          </node>
          <node class="string" name="host">
            <node-attribute name="value" value="localhost"/>
          </node>
          <node class="integer" name="port">
            <node-attribute name="value" value="7449"/>
          </node>
          <node class="integer" name="maxBinaryMessage">
            <node-attribute name="value" value="32894"/>
          </node>
          <node class="integer" name="maxIdleTime">
            <node-attribute name="value" value="90000"/>
          </node>
          <node class="integer" name="watchdogTimeout">
            <node-attribute name="value" value="120000"/>
          </node>

I removed

          <node class="boolean" name="virtualDesktopEnabled">
            <node-attribute name="value" value="TRUE"/>
          </node>

Sergey Ivanovskiy wrote:

I removed
[...]
from these tested configurations in order to use the standard hotel_gui directory settings, but the case is the same port=7449 works for the embedded client, but port=0 doesn't .

Have you imported the root CA into the browser? Is the https connection green in the left-side part of the URL?

I did not import CA into the browser, it seems that in the Firefox the icon looks like an orange triangle with an exclamation sign, but the Chrome logs insecure connection response from ehotel.js:529 GET https://localhost:41337/index.html?token=021c63c8592056a8866f4bf7ebd925d6 net::ERR_INSECURE_RESPONSE. Thank you, it helps to detect this problem with the certificate that appears again.

Looking into these

webSocketTimeout     integer     -1     no     Web socket timeout (in milliseconds). This is a timeout after which the web socket is closed .
watchdogTimeout     integer     -1     no     Watchdog timeout (in milliseconds). After the web socket is closed, the watchdog will wait at least that certain amount of time and, if the web socket was connected again, it will decide to terminate the FWD client, as the remote user is no longer connected to it. A value of -1 will mean the remote FWD client will be terminated as soon as the web socket is closed.

Web sockets use keep-alive HTTP connections, but does it mean that they leave forever or there exists a default timeout that is set by a particular web server?

I don't know the answer to this. How critical is it to our delivery today?

Can you provide a status of things?

If Hynek or Constantin need to help they will. We have to get this fixed, tested and into the trunk as early today as possible.

Greg Shah wrote:

I don't know the answer to this. How critical is it to our delivery today?

Can you provide a status of things?

I tested with hotel_gui it seems that there are no regressions in this code and there are no new issues and now I am doing ClientsToPortsGenerator utility to create map.clients-to-backends file that is referenced by the Apache configuration https://proj.goldencode.com/issues/2683#note-54.

If Hynek or Constantin need to help they will. We have to get this fixed, tested and into the trunk as early today as possible.

I didn't run ChUI regression tests.

Sergey Ivanovskiy wrote:

I didn't run ChUI regression tests.

I'll start these now.

Please run the ChUI regression testing immediately. The CTRL-C tests will be quite important.

Constantin: Thanks!

Thanks, I need this help to run 2683a at least once manually with hotel_gui using the embedded and web clients taking into account that it is required to change directory settings according 118, 119 and 120.

Sergey Ivanovskiy wrote:

Thanks, I need this help to run 2683a at least once manually with hotel_gui using the embedded and web clients.

I can help with this one. Is this for regression test? In other words is the reverse proxy setup needed?

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Thanks, I need this help to run 2683a at least once manually with hotel_gui using the embedded and web clients.

I can help with this one. Is this for regression test? In other words is the reverse proxy setup needed?

Thank you, for regression only, please take into account that it is required to add this new settings to the directory for hotel GUI (notes 118, 119, 120)

          <node class="container" name="002275">
            <node class="resource" name="resource-instance">
              <node-attribute name="reftype" value="TRUE"/>
              <node-attribute name="reference" value="com.goldencode.p2j.main.WebClientLauncher"/>
            </node>
            <node class="netRights" name="rights">
              <node-attribute name="permissions" value="'0101'B"/>
            </node>
            <node class="strings" name="subjects">
              <node-attribute name="values" value="all_others"/>
            </node>
          </node>
          <node class="container" name="002276">
            <node class="resource" name="resource-instance">
              <node-attribute name="reftype" value="TRUE"/>
              <node-attribute name="reference" value="com.goldencode.p2j.main.WebClientRegistrar"/>
            </node>
            <node class="netRights" name="rights">
              <node-attribute name="permissions" value="'0101'B"/>
            </node>
            <node class="strings" name="subjects">
              <node-attribute name="values" value="all_others"/>
            </node>
          </node>

Code Review Task branch 2683a Revision 11177

I like the changes.

I have only one question: is it possible to hit any of the WebClientsManager APIs during authentication processing? We have found this problem in other web-sockets code:

 * IMPORTANT: Do not use logging anywhere inside this class. When an authentication plugin
 * is running during session establishment with the P2J server using logging no messages are 
 * written into the log and the application becomes deadlocked.

You can search on this text to find the files affected. Please make consider if this can affect WebClientsManager (since it now uses logging).

Otherwise, if testing shows everything working properly, then we are good to go for a merge to trunk.

Sergey Ivanovskiy wrote:

Hynek Cihlar wrote:

Sergey Ivanovskiy wrote:

Thanks, I need this help to run 2683a at least once manually with hotel_gui using the embedded and web clients.

I can help with this one. Is this for regression test? In other words is the reverse proxy setup needed?

Thank you, for regression only, please take into account that it is required to add this new settings to the directory for hotel GUI (notes 118, 119, 120)

The hotel_gui app runs OK in both the virtualdesktop and embedded modes. Are there any particular tests you would like to run?

Thank you, I don't know that it seems that if the ports range is restricted, then it can be a weak place. I tested when ports are exhausted and clients are refreshed, closed or logged out, but it seems working.

Sergey Ivanovskiy wrote:

Thank you, I don't know that it seems that if the ports range is restricted, then it can be a weak place. I tested when ports are exhausted and clients are refreshed, closed or logged out, but it seems working.

Does this mean with restricted ports and the reverse proxy, Hotel GUI works OK?

Make sure that we are testing in 2 modes:

1. Through a reverse proxy.
2. Direct access.

Constantin Asofiei wrote:

Sergey Ivanovskiy wrote:

Thank you, I don't know that it seems that if the ports range is restricted, then it can be a weak place. I tested when ports are exhausted and clients are refreshed, closed or logged out, but it seems working.

Does this mean with restricted ports and the reverse proxy, Hotel GUI works OK?

It seems ok, tested them via Apache proxy server and run clients that are under this reverse proxy and clients that have direct access simultaneously.

But it will be great if the Apache environment manual tests will be approved and tested on the different host machine.

Constantin, could you help with this. I will try to add node to the directory using directory API, is it a correct way?

ds.addNode("/server/default/webClient/portsRange", "container", null);

Exception in thread "main" java.lang.RuntimeException: addNode() failed for the /server/default/webClient/portsRange node
at com.goldencode.p2j.main.ClientsToPortsGenerator.addNode(ClientsToPortsGenerator.java:346)
at com.goldencode.p2j.main.ClientsToPortsGenerator.generate(ClientsToPortsGenerator.java:163)
at com.goldencode.p2j.main.ClientsToPortsGenerator.main(ClientsToPortsGenerator.java:468)

Sergey Ivanovskiy wrote:

Constantin, could you help with this. I will try to add node to the directory using directory API, is it a correct way?
[...]

What do you need this for? You can debug and step into DirectoryService.addNode and see where it fails. Do you have this ds.addNode call in a ds.openBatch(nodeBasePath); and ds.closeBatch(true) bracket? (nodeBasePath is /server/default/webClient).

Thank you, I read docs and thought that openBatch brackets are used only for security nodes.

The clients to ports mapping tool works properly, only it creates case insensitive node name like 'portsrange' instead of 'portsRange'. It is due to IdUtils.normalize(nodeId)

Can I commit com.goldencode.p2j.main.ClientsToPortsGenerator into 2683a?

Sergey Ivanovskiy wrote:

Can I commit com.goldencode.p2j.main.ClientsToPortsGenerator into 2683a?

Yes.

Committed revision 11178, sorry for delay, during tests found that nodes are not updated if it is necessarily to satisfy (to >= from) and fixed it by placing them in the batch brackets to lock a node for updates.

Committed revision 11179 fixed java docs.

I just imported hotel server der certificate into Firefox and got embedded clients working properly.

Greg, if you need to proxy this url https://localhost:8443/, then it should be added separetely, like for this url https://localhost:7443

ProxyPass /gui  https://localhost:7443/gui
ProxyPassReverse /gui https://localhost:7443/gui

ProxyPass /embedded  https://localhost:8443
ProxyPassReverse /embedded https://localhost:8443

Yes, I will need to proxy both 8443 and 7443. 8443 is the really important one.

Is there anything else needed?

Sergey Ivanovskiy wrote:

Greg, if you need to proxy this url https://localhost:8443/, then it should be added separetely, like for this url https://localhost:7443
[...]

There is a issue with dojo loader if we will be proxy https://localhost:8443/, because it works by adding <script> nodes to the JS client DOM and it creates problems for correct mappings.

I guess that we need to fix dojo configuration settings.

         var dojoConfig =
         {
            baseUrl : "./",
            tlmSiblingOfDojo : false,
            packages : [
                        { name: "dojo", location: "dojo-release-1.10.0/dojo" }
                       ]
         };

All access is through port 443 right? So that is the only port exposed via the firewall?

What are the starting URLs for embedded mode and for virtual desktop mode?

Greg Shah wrote:

All access is through port 443 right? So that is the only port exposed via the firewall?

What are the starting URLs for embedded mode and for virtual desktop mode?

For virtual desktop mode it is https://forwardedhost/gui
For embedded client it should be https://forwardedhost/embedded but I have not fixed the correct mapping for this 3 levels proxy<->web-server<->web-client

Sergey Ivanovskiy wrote:

Greg Shah wrote:

All access is through port 443 right? So that is the only port exposed via the firewall?

What are the starting URLs for embedded mode and for virtual desktop mode?

For embedded client it should be https://forwardedhost/embedded but I have not fixed the correct mapping for this 3 levels proxy<->web-server<->web-client

At first it needs to change dojo config that was set by index.html to

      <script>
         var dojoConfig =
         {
            baseUrl : "./embedded",
            tlmSiblingOfDojo : false,
            packages : [
                        { name: "dojo", location: "dojo-release-1.10.0/dojo" }
                       ]
         };
      </script>

  /**
    * Log into the client driver in order to load the embedded, converted ABL content.
    *
    * @param   {string} u
    *          User name.
    * @param   {string} p
    *          Password.
    */
   me.login = function(u, p)
   {
      var ifr = document.getElementById("embeddedP2J");

      fwd_u = u;
      fwd_p = p;

      if (ifr.src.startsWith("https://"))
      {
         var msgId = getNextMessageId();

         var args = 
         {
            "messageId": msgId.toString(),
            "username" : fwd_u,
            "password" : fwd_p
         };

         p2j.embedded.invokeProcedure(null, "doLogin", args, null);
      }
      else
      {
         var xmlHttp = new XMLHttpRequest();
         xmlHttp.onreadystatechange = function()
         { 
            if (xmlHttp.readyState == 4 && xmlHttp.status == 200)
            {
               var fwd_url = xmlHttp.responseText;

               if (fwd_url.startsWith("null"))
               {
                  showError({ "error": "Could not connect to the FWD server!"} );
               }
               else
               {
                  ifr.src = fwd_url;

                  $("#p2jLoginContainer").hide();

                  // the main view is transparent at this point; however, we need to realize it here to
                  // work around a firefox bug (https://bugzilla.mozilla.org/show_bug.cgi?id=941146),
                  // which we would otherwise hit due to FWD's use of an unrealized canvas to calculate
                  // some font metrics
                  $("#mainView").show();

                  // mostly hide background image with a nearly opaque overlay
                  $("#bgFader").show().css("opacity", 0.87);

                  p2j.embedded.init("embeddedP2J", "embeddedP2JOverlay", "embedded-overlay");
                  p2j.embedded.setMessageCallback(ehotel.messageCallback);
                  p2j.embedded.setPageLoadedCallback(ehotel.pageLoaded);
                  p2j.embedded.setPageClosedCallback(ehotel.pageClosed);
                  p2j.embedded.setSocketDisconnectedCallback(ehotel.socketDisconnected);
                  p2j.embedded.setSocketReconnectedCallback(ehotel.socketReconnected);
               }
            }
         }

         xmlHttp.open("GET", "/launch", true); 
         xmlHttp.send(null);
      }
   };

Sergey Ivanovskiy wrote:

At first it needs to change dojo config that was set by index.html to

Are these changes compatible with non-proxy (normal) mode for embedded Hotel GUI?

in order to load scripts and the second issue with this incorrect POST request
[...]

The /launch in xmlHttp.open("GET", "/launch", true); is used to start the FWD embedded in the iframe (see EmbeddedWebAppDriver$EmbeddedPageHandler.handle).

Yes, I understand, but there are no ways, we need to change index.html and ehotel.js. It should take into account window.location to access /launch. Otherwise we can't create working example even using Apache html rewrite module since it can't handle such coded url mappings.

Sergey Ivanovskiy wrote:

Yes, I understand, but there are no ways, we need to change index.html and ehotel.js. It should take into account window.location to access /launch. Otherwise we can't create working example even using Apache html rewrite module since it can't handle such coded url mappings.

OK, please fix the /launch, too.

Any code changes to embedded mode will need to be applied to the customer POC project too.

Greg Shah wrote:

Any code changes to embedded mode will need to be applied to the customer POC project too.

Greg, does it need to do this work today?

Sergey Ivanovskiy wrote:

Greg Shah wrote:

Any code changes to embedded mode will need to be applied to the customer POC project too.

Greg, does it need to do this work today?

Yes, it is that customer POC code that we are having to deliver today.

Is there any remaining known issue with 2683a?

I understand that the POC embedded code needs changes. Is there anything else?

Besides the ChUI regression testing, is there any other testing that is yet to be done?

Finally: do these scenarios fully work with a single FWD server, reverse proxy and embedded server configuration?

1. reverse proxy to GUI embedded mode

user <--https_port_443-- apache_reverse_proxy --https_port_8443--> embedded_mode_web_server <--https_port_7443--> fwd_server
                                          |                                                                         |
                                          --https_wss_port_XYZ--> embedded_mode_web_client <-----spawn---------------

2. reverse proxy to GUI virtual desktop mode

user <--https_port_443-- apache_reverse_proxy --https_port_7443--> fwd_server ---------------------------
                                          |                                                             |
                                          --https_wss_port_XYZ--> embedded_mode_web_client <-----spawn---

3. direct to GUI embedded mode

user <--https_port_8443--> embedded_mode_web_server <--https_port_7443--> fwd_server----------------------
  |                                                                                                      |
  -------------------------------------------https_wss_port_XYZ--> embedded_mode_web_client <-----spawn---

4. direct to GUI virtual desktop

user <--https_port_7443--> fwd_server --------------------------------------------------------------------
  |                                                                                                      |
  -------------------------------------------https_wss_port_XYZ--> embedded_mode_web_client <-----spawn---

In cases 1 and 2, port XYZ can be any of these:

• dynamically allocated from a port range
• a single fixed port (single-user only)

In cases 3 and 4, port XYZ can be:

• dynamically allocated from any open port on the system (port == 0 in the directory)
• dynamically allocated from a port range
• a single fixed port (single-user only)

Greg Shah wrote:

Is there any remaining known issue with 2683a?

I understand that the POC embedded code needs changes. Is there anything else?

Besides the ChUI regression testing, is there any other testing that is yet to be done?

Finally: do these scenarios fully work with a single FWD server, reverse proxy and embedded server configuration?

[...]

In cases 1 and 2, port XYZ can be any of these:

• dynamically allocated from a port range
• a single fixed port (single-user only)

It seems that a single fixed port is the same as dynamically allocated from a degraded port range. I didn't provide configuration for a single fixed port and even it doesn't work because of the design. If the restricted port range is not given and the reverse proxy is present then WebClientsManager.allocateClient throws an exception. Please look at this code

      if (viaProxyServer && !portsRestricted)
      {
         throw new IllegalStateException("Correct range: " 
                  + "'webClient/portsRange/from' < 'webClient/portsRange/to' must be provided");
      }

In cases 3 and 4, port XYZ can be:

• dynamically allocated from any open port on the system (port == 0 in the directory)
• dynamically allocated from a port range
• a single fixed port (single-user only)

All these cases I tested on my host but no one else had tested them.

It seems that a single fixed port is the same as dynamically allocated from a degraded port range. I didn't provide configuration for a single fixed port and even it doesn't work because of the design. If the restricted port range is not given and the reverse proxy is present then WebClientsManager.allocateClient throws an exception. Please look at this code

OK, this is fine. As long as the port range works with both cases 1 and 2, then it is good.

My next step is to get the POC directory.xml configured properly.

If I understand correctly, I only need to do these two things:

• Run ClientsToPortsGenerator. This will create the mapping text file and will enable the port range in the directory.
• Add the ACL for WebClientsManager.

Is any other directory.xml change needed?

It seems that there no another changes. I updated this poc and started postgresql server, now running ant deploy.all it takes 7-8 minutes, then I will change the required embedded web server files, the same as for hotel_gui embedded and will test these changes.

I need help with poc setup, I run ant deploy.all and got

nConnection(LogicalConnectionImpl.java:297)
     [java]     ... 22 more
     [java] Caused by: com.mchange.v2.resourcepool.CannotAcquireResourceException: A ResourcePool could not acquire a resource from its primary factory or source.
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.awaitAvailable(BasicResourcePool.java:1469)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.prelimCheckoutResource(BasicResourcePool.java:644)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.checkoutResource(BasicResourcePool.java:554)
     [java]     at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool.checkoutAndMarkConnectionInUse(C3P0PooledConnectionPool.java:758)
     [java]     at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool.checkoutPooledConnection(C3P0PooledConnectionPool.java:685)
     [java]     ... 26 more
     [java] Caused by: org.postgresql.util.PSQLException: FATAL: password authentication failed for user "fwd_user" 
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.doAuthentication(ConnectionFactoryImpl.java:446)
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:220)
     [java]     at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:55)
     [java]     at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:219)
     [java]     at org.postgresql.Driver.makeConnection(Driver.java:407)
     [java]     at org.postgresql.Driver.connect(Driver.java:275)
     [java]     at com.mchange.v2.c3p0.DriverManagerDataSource.getConnection(DriverManagerDataSource.java:175)
     [java]     at com.mchange.v2.c3p0.WrapperConnectionPoolDataSource.getPooledConnection(WrapperConnectionPoolDataSource.java:220)
     [java]     at com.mchange.v2.c3p0.WrapperConnectionPoolDataSource.getPooledConnection(WrapperConnectionPoolDataSource.java:206)
     [java]     at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool$1PooledConnectionResourcePoolManager.acquireResource(C3P0PooledConnectionPool.java:203)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.doAcquire(BasicResourcePool.java:1138)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.doAcquireAndDecrementPendingAcquiresWithinLockOnSuccess(BasicResourcePool.java:1125)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.access$700(BasicResourcePool.java:44)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool$ScatteredAcquireTask.run(BasicResourcePool.java:1870)
     [java]     at com.mchange.v2.async.ThreadPoolAsynchronousRunner$PoolThread.run(ThreadPoolAsynchronousRunner.java:696)
     [java] Jun 30, 2017 10:54:32 PM com.mchange.v2.log.slf4j.Slf4jMLog$Slf4jMLogger$WarnLogger log
     [java] WARNING: com.mchange.v2.resourcepool.BasicResourcePool$ScatteredAcquireTask@7dc2a0df -- Acquisition Attempt Failed!!! Clearing pending acquires. While trying to acquire a needed new resource, we failed to succeed more than the maximum number of allowed acquisition attempts (30). Last acquisition attempt exception: 
     [java] org.postgresql.util.PSQLException: FATAL: password authentication failed for user "fwd_user" 
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.doAuthentication(ConnectionFactoryImpl.java:446)
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:220)
     [java]     at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:55)
     [java]     at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:219)
     [java]     at org.postgresql.Driver.makeConnection(Driver.java:407)
     [java]     at org.postgresql.Driver.connect(Driver.java:275)
     [java]     at com.mchange.v2.c3p0.DriverManagerDataSource.getConnection(DriverManagerDataSource.java:175)
     [java]     at com.mchange.v2.c3p0.WrapperConnectionPoolDataSource.getPooledConnection(WrapperConnectionPoolDataSource.java:220)
     [java]     at com.mchange.v2.c3p0.WrapperConnectionPoolDataSource.getPooledConnection(WrapperConnectionPoolDataSource.java:206)
     [java]     at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool$1PooledConnectionResourcePoolManager.acquireResource(C3P0PooledConnectionPool.java:203)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.doAcquire(BasicResourcePool.java:1138)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.doAcquireAndDecrementPendingAcquiresWithinLockOnSuccess(BasicResourcePool.java:1125)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.access$700(BasicResourcePool.java:44)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool$ScatteredAcquireTask.run(BasicResourcePool.java:1870)
     [java]     at com.mchange.v2.async.ThreadPoolAsynchronousRunner$PoolThread.run(ThreadPoolAsynchronousRunner.java:696)
     [java] 
     [java] Jun 30, 2017 10:54:32 PM com.mchange.v2.log.slf4j.Slf4jMLog$Slf4jMLogger$WarnLogger log
     [java] WARNING: Having failed to acquire a resource, com.mchange.v2.resourcepool.BasicResourcePool@f0da945 is interrupting all Threads waiting on a resource to check out. Will try again in response to new client requests.
     [java] Jun 30, 2017 10:54:32 PM com.mchange.v2.log.slf4j.Slf4jMLog$Slf4jMLogger$WarnLogger log
     [java] WARNING: com.mchange.v2.resourcepool.BasicResourcePool$ScatteredAcquireTask@6bb42a93 -- Acquisition Attempt Failed!!! Clearing pending acquires. While trying to acquire a needed new resource, we failed to succeed more than the maximum number of allowed acquisition attempts (30). Last acquisition attempt exception: 
     [java] org.postgresql.util.PSQLException: FATAL: password authentication failed for user "fwd_user" 
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.doAuthentication(ConnectionFactoryImpl.java:446)
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:220)
     [java]     at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:55)
     [java]     at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:219)
     [java]     at org.postgresql.Driver.makeConnection(Driver.java:407)
     [java]     at org.postgresql.Driver.connect(Driver.java:275)
     [java]     at com.mchange.v2.c3p0.DriverManagerDataSource.getConnection(DriverManagerDataSource.java:175)
     [java]     at com.mchange.v2.c3p0.WrapperConnectionPoolDataSource.getPooledConnection(WrapperConnectionPoolDataSource.java:220)
     [java]     at com.mchange.v2.c3p0.WrapperConnectionPoolDataSource.getPooledConnection(WrapperConnectionPoolDataSource.java:206)
     [java]     at com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool$1PooledConnectionResourcePoolManager.acquireResource(C3P0PooledConnectionPool.java:203)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.doAcquire(BasicResourcePool.java:1138)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.doAcquireAndDecrementPendingAcquiresWithinLockOnSuccess(BasicResourcePool.java:1125)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool.access$700(BasicResourcePool.java:44)
     [java]     at com.mchange.v2.resourcepool.BasicResourcePool$ScatteredAcquireTask.run(BasicResourcePool.java:1870)
     [java]     at com.mchange.v2.async.ThreadPoolAsynchronousRunner$PoolThread.run(ThreadPoolAsynchronousRunner.java:696)
     [java] 
     [java] Jun 30, 2017 10:54:32 PM com.mchange.v2.log.slf4j.Slf4jMLog$Slf4jMLogger$WarnLogger log
     [java] WARNING: com.mchange.v2.resourcepool.BasicResourcePool$ScatteredAcquireTask@341915f0 -- Acquisition Attempt Failed!!! Clearing pending acquires. While trying to acquire a needed new resource, we failed to succeed more than the maximum number of allowed acquisition attempts (30). Last acquisition attempt exception: 
     [java] org.postgresql.util.PSQLException: FATAL: password authentication failed for user "fwd_user" 
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.doAuthentication(ConnectionFactoryImpl.java:446)
     [java]     at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:220)
     [java]     at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:55)

Are you on revision 53 of the POC project? You are probably on rev 52 which was hard coded to PostgreSQL.

Try rev 53. Please note that there are some values in the directory that have to be hand-modified to make it work:

UNIX_USER_NAME
PROJECT_PARENT_PATH

Ok, but it worked on my host with Posgresql before that.

Sergey Ivanovskiy wrote:

Ok, but it worked on my host with Posgresql before that.

Hotel was setup for postgresql. But the customer POC code was only ever working on H2 until a couple of days ago. We've backed that out (as the default) temporarily.

For now, just use the H2 path. The work in this task doesn't need postgresql.

Thank you, the poc is built now. I forgot that the embedded client needs a direct access too, but I changed its root consistently to /embedded, so the web server also need to be changed. I tested via proxy. It should not be a problem, I would like to change a direct access to https://localhost:8443/embedded

It should be a problem, I would like to change a direct access to https://localhost:8443/embedded

Go ahead.

Greg Shah wrote:

It should be a problem, I would like to change a direct access to https://localhost:8443/embedded

Go ahead.

It is stupid, I fooled myself, it doesn't need to do any changes, it is enough to change an access url to https://localhost for the embedded and to change the reverse proxy to

ProxyPass / https://localhost:8443/
ProxyPassReverse / https://localhost:8443/

Sergey Ivanovskiy wrote:

Greg, Constantin these changes work with the Apache configuration provided by note 152, I did not use HTML rewrite module to fix urls inside html, because it is impossible for our case. Please review this fix, it works :)

Don't mind, these changes are not required at all. We can simply have another url and different proxy settings. For the embedded client it can be https://localhost/
and mapping can be

ProxyPass / https://localhost:8443/
ProxyPassReverse / https://localhost:8443/

Can you check in your changes to the POC embedded code?

Greg Shah wrote:

Can you check in your changes to the POC embedded code?

It doesn't need to change the source code if you are agree to use https://localhost/ for https://localhost:8443/. Please recall the account/password for poc embedded client.

Don't mind, I found REDACTED/REDACTED

Do you need my help with Apache configuration?

Yes.

I'm about to work on that next.

Please make a list of the following:

1. Packages to install to get Apache and all modules in place.
2. Command line module enable/site enable statements.
3. Specific configuration files OR changes to configuration files that are needed.
4. Whatever else needs to be done that I'm not thinking of.

I need a kind of "cookbook" for getting this going. I will take that and script it in Ansible.

Also, please update the directory.xml to have the right ACL and the new port range support for 10 clients.

Include the client match list in the apache cfg.

Committed revision 55 modified poc directory.xml.

Also after your configuration has been done, you can check the installed modules by running

sudo a2query -m

access_compat (enabled by maintainer script)
alias (enabled by maintainer script)
setenvif (enabled by maintainer script)
headers (enabled by site administrator)
proxy_connect (enabled by site administrator)
deflate (enabled by maintainer script)
filter (enabled by maintainer script)
authz_user (enabled by maintainer script)
proxy_wstunnel (enabled by site administrator)
dir (enabled by maintainer script)
authz_core (enabled by maintainer script)
proxy_html (enabled by site administrator)
auth_basic (enabled by maintainer script)
negotiation (enabled by maintainer script)
env (enabled by maintainer script)
authn_file (enabled by maintainer script)
proxy (enabled by site administrator)
status (enabled by maintainer script)
authn_core (enabled by maintainer script)
proxy_http (enabled by site administrator)
socache_shmcb (enabled by site administrator)
rewrite (enabled by site administrator)
mpm_prefork (enabled by site administrator)
autoindex (enabled by maintainer script)
authz_host (enabled by maintainer script)
mime (enabled by maintainer script)
ssl (enabled by site administrator)

Please notice modules that enabled by site administrator. All these modules must be installed if you would like to get the similar configuration on your host.

Do all of these get installed with the apache2 package?

Greg Shah wrote:

Do all of these get installed with the apache2 package?

I am not sure, but it seems that they are included in this package.

openssl package must be installed.

At first it is required to create ssl virtual host and it seems that I followed this article https://www.digitalocean.com/community/tutorials/how-to-create-a-ssl-certificate-on-apache-for-ubuntu-14-04

Can't we just pass through the SSL of the FWD server's? We don't want to generate yet another certificate that is only used for reverse proxy anyway (a pass through);

Yes, I think it helps to fix embedded client's certificate issue. It seems that you wrote about it when preparing an access to demo.

I setup certificates for the FWD server and the embedded web server for demo.goldencode.com. Those just replaced the certs in the poc project.

I did not setup an Apache proxy configuration with certificates. What I am hoping is that Apache can be setup to just pass through the SSL.

If that isn't possible, then we would want to re-use the certs that are already in the poc project.

I followed https://www.digitalocean.com/community/tutorials/how-to-use-apache-http-server-as-reverse-proxy-using-mod_proxy-extension.

Planning to log out sooner.

Constantin: Is the ChUI regression testing still running?

Greg Shah wrote:

Constantin: Is the ChUI regression testing still running?

CTRL-C I think is passed (cumulative, 3-way and 2-way sets pass in different runs, couldn't get a clean run...).

main part is still running.

main part passed, too.

Are you OK with merging to trunk?

Also: I plan to enable virtual desktop in the poc directory. Is there any reason to disable it?

Greg Shah wrote:

Are you OK with merging to trunk?

Yes, I don't see any issues.

Also: I plan to enable virtual desktop in the poc directory. Is there any reason to disable it?

I don't see one, but I'll do a quick test.

I'll test it now.

Please consider the cert re-gen issue.

Virtual desktop mode works in the POC. AND because of the port range support, I can get multiple simultaneous sessions going to the same server!

If we can provide the mechanism to regen the cert for an arbitrary hostname, then we can meet our deadline.

Greg Shah wrote:

If we can provide the mechanism to regen the cert for an arbitrary hostname, then we can meet our deadline.

• the "embedded" process - this is not linked to FWD, I've added it in the directory just to be able to use the FWD tool to generate the certificate
• the "server" process - in use by the FWD iframe. This is not standalone, so it can't be easily switched.

I can add some property to the process account (or in webClient/<process-id>/<host-name>) to be able to configure the common name, or otherwise change SSLCertGenUtil to ask for the common name to be used.

But regardless, after the certs are re-issued, the embedded_server_cfg_launch.sh (and .cmd) will need to be reconfigured with the proper passwords. And when these are set, some characters will require to be escaped (% in Windows and ), $ and others in Linux).

Ideally, I think we would want the web certificates to not be linked to any FWD internals (i.e. not use the server certificate), and be just a standalone certificate, which can be managed externally and just configured in the directory.

So, the fast way at this time I think is to use the webClient/<process-id>/<host-name> approach.

I am already automatically updating the directory.xml placeholders using the Ansible scripting. We can add another placeholder and it is easy to handle.

Likewise, we can pass a name as part of the ansible command line (I am already passing many other parameters this way). And we can pass this name to SSLCertGenUtil from Ansible if that is helpful.

I can make similar edits to embedded_server_cfg_launch.sh BUT why not reuse the same passwords? This is the simplest solution and will work for the poc.

Greg Shah wrote:

I am already automatically updating the directory.xml placeholders using the Ansible scripting. We can add another placeholder and it is easy to handle.

Yes, I'll add a directory configuration under company/hostnames/<process>/<host> , with <host> needing to be a placeholder.

Likewise, we can pass a name as part of the ansible command line (I am already passing many other parameters this way). And we can pass this name to SSLCertGenUtil from Ansible if that is helpful.

SSLCertGenUtil reads a lot of input from STDIN. Can this be managed in ansible? Otherwise I'll need to read the input from the arguments (or use a 'auto' mode).

I can make similar edits to embedded_server_cfg_launch.sh BUT why not reuse the same passwords? This is the simplest solution and will work for the poc.

We can re-use the passwords used to encrypt the private keys in the directory. But it will be used to encrypt both the private key and the key store.

Otherwise I'll need to read the input from the arguments

Yes, let's make these arguments. I can pass them in as needed.

We can re-use the passwords used to encrypt the private keys in the directory. But it will be used to encrypt both the private key and the key store.

This is fine for tonight.

Greg, I'm almost done with this: do you want to add 're-use root CA', too? Because otherwise this would need to be re-installed.

Yes.

Greg Shah wrote:

Yes.

OK, then I'll need to save its encryption password for its private key in the directory.

Or we can pass it in as a parameter.

There is this option webClient/portsRange/forwardedHost that can be important to set in Amazon environment. (https://proj.goldencode.com/issues/3236#note-47) It is a frontend proxy server host (external host that is visible outside the private network).

          <node class="container" name="portsrange">
            <node class="integer" name="from">
              <node-attribute name="value" value="7449"/>
            </node>
            <node class="integer" name="to">
              <node-attribute name="value" value="7459"/>
            </node>
            <node class="string" name="nameprefix">
              <node-attribute name="value" value="client"/>
            </node>
            <node class="string" name="forwardedHost">
              <node-attribute name="value" value="www.goldencode.com"/>
            </node>
<!-- it can be skipped since its value is always 'https' -->
            <node class="string" name="forwardedProto">
              <node-attribute name="value" value="https"/>
            </node>
          </node>

Greg, 2683a rev 11180 contains the certificate-related changes (and the chrome 'subject-alternate-name' fix, too). Also, POC rev 60 contains the new certificates.

1. in directory.xml, search for hostnames and replace localhost with your placeholder
2. execute SSLCertGenUtil with these arguments: directory.xml 2048 65337 yes yes fBMaU6k(r6<G^tUeZEg=BW48z3mM4M29 yes no yes no yes yes srv-certs.store yes root-ca-pk.store, from deploy/server.
3. execute this command in deploy/server to generate the .der:
   

   keytool -exportcert -alias hotel-root -keystore srv-certs.store -file hotel-root.der
   

   
   and use 169ghwa^G0f%O9p<`feODy1q7NTFWI8j as password (will be read from STDIN).
4. import the hotel-root.der into the browser(s).
5. re-install spawner.

After 2683a rev 11180, each time SSLCertGenUtil is executed with the above parameters, the root CA will be re-used and the encryption password will be used for the stores.

Greg, I think I gave you the wrong password, and I need to re-generate them again - I get the same now (even from Eclipse), something messed up when I committed the directory.

2683a was merged to trunk as revision 11154.