-
Notifications
You must be signed in to change notification settings - Fork 116
Web sockets for save&restore #3389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
core/websocket/src/main/java/org/phoebus/core/websocket/WebSocketClient.java
Outdated
Show resolved
Hide resolved
abrahamwolk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two questions:
-
Suppose the connection is lost between the client and the server, and suppose that changes were made to a snapshot before the connection was re-established. Will the client be notified of the changes that were made while the connection was lost?
-
How is the situation dealt with when a snapshot is simultaneously being worked on by two clients in possibly non-compatible ways? For instance, one client may remove a folder, while another client may simultaneously add a node to the folder, or rename the folder in question.
| List<String> selectedNodeIds = | ||
| ((List<Node>) selectedNodes).stream().map(Node::getUniqueId).collect(Collectors.toList()); | ||
| JobManager.schedule("copy nodes", monitor -> { | ||
| JobManager.schedule("Copy odes", monitor -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the first argument to JobManager.schedule() be "Copy nodes"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed
| * @param snapshotNode An existing {@link Node} of type {@link NodeType#SNAPSHOT} | ||
| */ | ||
| public void loadSnapshot(Node snapshotNode) { | ||
| public synchronized void loadSnapshot(Node snapshotNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is synchronized, but the call to loadSnapshotInternal() will most likely return before the job scheduled by JobManager.schedule() has run, which in turn most likely will return before the function submitted to Platform.runLater() has run. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, need not be synchronized. Leftover from testing.
| tabGraphicImageProperty.set(ImageRepository.SNAPSHOT); | ||
| } | ||
| } | ||
| //WebSocketClientService.getInstance().addWebSocketMessageHandler(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix
|
|
||
| private void loadConfigurationData(Runnable completion) { | ||
| UI_EXECUTOR.execute(() -> { | ||
| public synchronized void loadConfiguration(final Node node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is synchronized, but the functions submitted to JobManager.schedule() and Platform.runLater() are run asynchronously and will most likely return after this method has returned. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, need not be synchronized. Leftover from testing.
|
|
regarding 2. |
+1 |
|
Incompatible updates can still happen even though updates to snapshots are relatively infrequent. E.g., a computer may be left unattended with unsaved changes for some amount of time, or a client with unsaved changes may be disconnected for some amount of time because a computer was suspended. One idea, that does not use locks, could be that each revision of a snapshot is given its own unique ID (perhaps implemented using a counter), and on writing, the unique ID of the revision that is overridden is compared on the server against the unique ID of the revision that the edit is based on. If they don't match, then the snapshot has been updated while editing took place, and an error or warning can be displayed. |
|
Save & restore has been in use for quite some time without anything preventing simultaneous edits. Further, currently there is no way to refresh data other than collapsing/expanding nodes in the tree view. On top of that, an object being edited must be closed and reopened to reflect changes. What web sockets add is a way to make sure the UI reflects changes made by others. Safeguarding against simultaneous edits is in my view not in scope for this PR and the introduction of web sockets. |
|
What will user A see if user A is editing a snapshot, and user B writes to the snapshot? |
|
Then user A will see the state saved by B and A's edits are lost. B's identity will be apparent from the updated UI. |
| @Override | ||
| public boolean handleTabClosed() { | ||
| saveLocalState(); | ||
| //saveLocalState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out line of code.
|
|
||
| private void handleWebSocketConnected() { | ||
| serviceConnected.setValue(true); | ||
| Platform.runLater(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call Platform.runLater() with a function that does nothing? Both here and in handleWebSocketDisconnected().
|
Interesting observation (at least on MacOS): disabling WiFi does not trigger a web socket disconnect event. Moreover, when WiFi is enabled again, the web socket remains operational. In any case, I have added a refresh of the UI when service is brought back on-line and client succeeds to reconnect. |
|
Based on observations in various test scenarios: in order to handle different kind of web socket connection issues, a ping/pong strategy is needed. Therefore the client will dispatch a ping message and consider the connection dead if a pong message is not received within three seconds. |
This PR adds a web sockets-based mechanism for save&restore whereby data changes on the service side are pushed as web socket messages to all connected clients.
On the Phoebus app side, the
core-websocketmodule adds a web socket client using the native Java APIs. Clients of theWebSocketClientonly need to specify a URI and a callback for receiving text messages. Optionally API clients may register callbacks to handle/debug connection and disconnection events.WebSocketClientshould be generic enough to support multiple use cases, e.g. logbook or alarm logger UI.WebSocketClientwill by default try to reconnect to the remote service in case the remote peer is shut down.The save&restore app makes use of the
WebSocketClientto update the UI based on whatever the service pushes as web socket messages. This way all clients should be able to reflect changes done by all users.