FWD » Core Development » User Interface

For additional details, please refer to entries #10428-24 through #10428-29

Hynek Cihlar wrote:

Code review 10485a.

Please remove PushMessagesWorker.session

If we remove the session, wouldn’t it also make sense to drop sessionInvalid and sessionGuard, since they only exist to manage the session state?

Hynek Cihlar wrote:

Code review 10485a.

Please remove PushMessagesWorker.session and the related methods.

[...]

Sergey, please review.

I will do this later but why not to change NetworkTestHandler? Is this issue is really related to #10428?
As this method is created for NetworkTestHandler usages and it was invoked via the different thread but all messages should be passed via PushMessagesWorker.

   public void sendBinaryMessageSync(byte[] message)
   {
      synchronized (lock)
      {
         try
         {
            if (session.isOpen())
            {
               session.getRemote().sendBytes(ByteBuffer.wrap(message));
            }
         }
         catch (IOException ex)
         {
            LOG.severe("", ex);
         }
      }
   }

Paula Păstrăguș wrote:

Hynek Cihlar wrote:

Code review 10485a.

Please remove PushMessagesWorker.session

If we remove the session, wouldn’t it also make sense to drop sessionInvalid and sessionGuard, since they only exist to manage the session state?

Hm, PushMessageWorker uses session for connection state control so it can't be removed. Ideally this should be moved to WebClientProtocol as well since the message sending is now there.

Please check whether the session state control doesn't pose another race condition issue.

   public void sendBinaryMessageSync(byte[] message)
   {
      synchronized (lock)
      {
         try
         {
            if (session.isOpen())
            {
               session.getRemote().sendBytes(ByteBuffer.wrap(message));
            }
         }
         catch (IOException ex)
         {
            LOG.severe("", ex);
         }
      }
   }

Greg, please review the code in the trunk, now Paula is trying to make this work via different threads.

Hynek / Sergey, I don’t think the solution I pushed is really viable. We’re synchronizing on the same lock that’s also used for reading keys, injecting keys, and other operations in WebClientProtocol. This can make the GUI feel sluggish, I noticed the slowdown when hovering over an image where a PS timer is defined to change it.

The race condition can only occur during a network test when a PS timer is active and its callback interacts with the WebSocket. I suggest to keep the retry mechanism for sending messages when an IllegalStateException is thrown.

Paula Păstrăguș wrote:

Hynek / Sergey, I don’t think the solution I pushed is really viable. We’re synchronizing on the same lock that’s also used for reading keys, injecting keys, and other operations in WebClientProtocol. This can make the GUI feel sluggish, I noticed the slowdown when hovering over an image where a PS timer is defined to change it.

I think we should support the current design that
session.getRemote().sendBytes is accessed via the dedicated thread created by PushMessagesWorker,
otherwise we should invent new conception with multi-threaded access to session.getRemote().sendBytes .

Sergey Ivanovskiy wrote:

Is this issue is really related to #10428?

Not exactly, but since the main discussion took place in that task, I thought it would be useful to reference it.

Paula Păstrăguș wrote:

I’ve attached a patch that switches the implementation to use a ReentrantLock. If this approach looks good, I’ll commit the changes and roll back the current solution.

Would this work for you? Hynek / Sergey, please take a look.

I think it is OK to use a shared lock object. Just don't make it static. Yes, currently it doesn't make any functional difference to make it non-static, but let's make the intention clear. It belongs to the particual web socket object so it should be expressed such in the code. This may solve potential future issues.

Also please update the javadocs of the changed constructors.

Shouldn't sendString be protected, too?

Hynek Cihlar wrote:

Shouldn't sendString be protected, too?

Indeed, it should have been protected too. I overlooked that detail.

Paula Păstrăguș wrote:

I’ve attached a patch that switches the implementation to use a ReentrantLock. If this approach looks good, I’ll commit the changes and roll back the current solution.

Would this work for you? Hynek / Sergey, please take a look.

Please make your changes in 10485a for review. Do you suppose to use PushMessageWorker.sendMessage from the different thread? It was supposed that PushMessageWorker.sendMessage was used from pushworker thread only.

Yes, sendMessage is only called from the push worker thread, so in practice it doesn’t really make sense to also guard sendString, since it’s used exclusively within that flow. My concern was more about future changes, if at some point another thread ends up calling sendString, having the lock in place would already prevent any issues.

Paula Păstrăguș wrote:

Yes, sendMessage is only called from the push worker thread, so in practice it doesn’t really make sense to also guard sendString, since it’s used exclusively within that flow. My concern was more about future changes, if at some point another thread ends up calling sendString, having the lock in place would already prevent any issues.

It is best to ensure that there is no way to use these methods which could break our synchronization.

Paula Păstrăguș wrote:

Committed as rev 16136.

Please review.

The changes look good to me. Sergey, what do you think?

Let me explain my point of view. Please look at the usage of MSG_GET_WEB_BASIC_PALETTE of WebClientProtocol.
GuiWebDriver.getBasicWebPalette sends a message via web socket and waits a result. In this situation we can said that the method synchronously returns the result. Compare this situation with WebClientProtocal.sendTextMessageSync(byte[] message) that is documented to send message synchronously but actually this message is just not queued via PushMessageWorker. Correct? This introduces confusions and makes PushMessageWorker useless because messages can be sent directly into the web socket. It seems for me that if NetworkTestHandler.startDownloadTest should measure the application download test, then it should use PushMessageWorker, but if this method should estimate indirectly the network speed with the server host, then it could use a new websocket channel.

The changes rev 16136 (10485a) should obviously fix the race condition. This method
public void sendTextMessageSync(String message) of WebClientProtocal has no usages and it seems that can be removed.

Sergey raises a valid point. Adding synchronization logic to cover up a flawed design is not a good approach.

The original intent of the test logic was to bypass PushMessagesWorker entirely and measure raw network performance with as little interference from the framework as possible.

The best solution would be to open a completely new socket for the throughput measurement. This would also eliminate the need for additional synchronization.

Sergey, do you agree with this approach?

Yes, I agree.

The network test is only a diagnostic tool. Real users will almost never use it. The websocket should only ever be created when the test is used, it should not be there under normal conditions.

I don't think we need any special retry processing, since this is not a production connection. A best efforts check on network throughput is all we need to do.

Paula Păstrăguș wrote:

Addressed points 2 and 3 in rev 16138.

I double checked the first point, the socket is already closed at the end of the test (see p2j.perf_tests.js, line 1913), and it is also closed when the test is canceled. Or perhaps I missed something, could you clarify if that’s the case?

OK, if it is closed from JS it is fine.

Please test the case when the network connection is briefly interrupted. It is OK for the test to fail, but the user should be able to continue with his work, also the user should be able to run another test.

Please fix the type "Clossing network websocket...".

When the web socket is closed should also the associated handlers be removed before the reference is set to null in p2j.networkSocket.close()?

Paula Păstrăguș wrote:

I’d like your input: should we leave the behavior as it is, or also display the Test abort message when the network WebSocket is closed after an interruption?

Paula, your implemented behavior is correct. Just please change the message so that it is clear the test failed due to connection error.

Code review.

When logging the websocket close/error event, do not log just the reason property, but the whole object: console.log("WebSocket closed.", event);.

The connection error is output to the test results from the websocket's close handler. Should it be output from error handler instead so that only connection errors are output?

Hynek Cihlar wrote:

The connection error is output to the test results from the websocket's close handler. Should it be output from error handler instead so that only connection errors are output?

I believe we should always log whenever the WebSocket is closed, whether it’s intentional (by design) or due to a connection interruption / other causes.

Also, no error is raised when the internet connection is interrupted and then restored. I’ll double-check, but as I remember, the WebSocket just closes without throwing any error.. a bit strange, but that’s how it behaves.

Paula Păstrăguș wrote:

Hynek Cihlar wrote:

The connection error is output to the test results from the websocket's close handler. Should it be output from error handler instead so that only connection errors are output?

I believe we should always log whenever the WebSocket is closed, whether it’s intentional (by design) or due to a connection interruption / other causes.

The thing is "Test failed: connection error" is output in all the cases (when close handler is invoked).

Hynek Cihlar wrote:

Paula Păstrăguș wrote:

Hynek Cihlar wrote:

The connection error is output to the test results from the websocket's close handler. Should it be output from error handler instead so that only connection errors are output?

I believe we should always log whenever the WebSocket is closed, whether it’s intentional (by design) or due to a connection interruption / other causes.

The thing is "Test failed: connection error" is output in all the cases (when close handler is invoked).

You may also check wasClean of the close event.

I think Test failed on its own is enough, no need to mention connection error, since the exact reason isn’t that important here. Right?

I noticed that I was assigning null to the handlers too early. What happened is that the cleanup ran right after calling close(), before the socket had actually finished closing, which meant the onclose event couldn’t fire properly. I’ve moved the cleanup into the onclose handler so it runs at the right time.

Paula Păstrăguș wrote:

I think Test failed on its own is enough, no need to mention connection error, since the exact reason isn’t that important here. Right?

If it failed, the user should know. The reason is useful I think.

Okay, so I can take the status code (per https://datatracker.ietf.org/doc/html/rfc6455#section-7.4.1) and map it to a user-friendly message?

Paula Păstrăguș wrote:

Okay, so I can take the status code (per https://datatracker.ietf.org/doc/html/rfc6455#section-7.4.1) and map it to a user-friendly message?

I think a generic description of an unexpected network error plus the code in the message area is OK.

Addressed review as rev 16140.

Hotel GUI smoke testing also passed with the last rev of 10485a.

I believe Hotel Gui and the large GUI application provide sufficient coverage. If there's anything else you'd like me to test, feel free to mention it. Otherwise, I'm good to proceed with the merge.