Wicket 6930 websocket improvements by reiern70 · Pull Request #479 · apache/wicket

reiern70 · 2021-11-07T09:58:58Z

This PR adds some extra functionality to WS implementation. Not sure if this can be done in a cleaner way or a more general one. This adds.

Allows to avoid sending "empty" web socket push messages (i.e. no components or scripts are added to it). The use case is user starts a background task and component listening for it are no longer in page. Thus events still go through a page but nothing is added to them.
The pageClass is added as a variable to WS page related key. Use case? You want to deliver to a class of pages... or you want to start a task and leave a page, or open a new page and still receive events..
I modified the examples to illustrate this.

reiern70 · 2021-11-07T09:12:19Z

@martin-g Not very sure about the pageCLass I have introduced. Maybe call this "context"? i.e. page class is just a. context?

martin-g

About the new pageClass attribute:

PageIdKey is no more accurate name, since it is not just the id anymore. But renaming it would be an API break
What if someone wants to do the same with a resource based WebSocket behavior ?

IMO we should move this logic at IKey level.
And if we can avoid class loading it would be better, but I am not sure how exactly to do it yet!

martin-g · 2021-11-08T09:29:54Z

+        progressUpdateTask.cancel();
+    }
+
+    public static JSR356Session getInstance() {


s/getInstance/get/

martin-g · 2021-11-08T09:30:43Z

+            return;
+        }
+        ScheduledExecutorService service = JSR356Application.get().getScheduledExecutorService();
+        progressUpdateTask = ProgressUpdater.start(getApplication(), getId(), service);


You can cache the value of JSR356Application.get() and reuse it instead of calling getApplication()

martin-g · 2021-11-08T09:31:46Z

+        add(new WebSocketBehavior()
+        {
+            @Override
+            protected void onConnect(ConnectedMessage message) {


this override is not really needed here

martin-g · 2021-11-08T09:32:44Z

+        progressUpdateTask = ProgressUpdater.start(getApplication(), getId(), service);
+    }
+
+    public synchronized void cancel() {


s/cancel/cancelTask/

martin-g · 2021-11-08T09:33:10Z

+                ProgressUpdater.ProgressUpdateTask progressUpdateTask = JSR356Session.getInstance().getProgressUpdateTask();
+                if (progressUpdateTask != null && progressUpdateTask.isRunning() && !progressUpdateTask.isCanceled())
+                {
+                    progressUpdateTask.cancel();


You can simplify it to JSR356Session.getInstance().cancelTask();

martin-g · 2021-11-08T09:49:48Z

+	interface IConnectionsFilter
+	{
+
+		boolean acceptConnection(String sessionId, IKey key);


rename to accept(...) ?

martin-g · 2021-11-08T09:51:36Z

 		this.pageId = Args.notNull(pageId, "pageId");
+		Args.notNull(pageClass, "pageClass");
+		try {
+			this.pageClass = (Class<IManageablePage>)Thread.currentThread().getContextClassLoader().loadClass(pageClass);


This should use IClassResolver

I have refactored code. Now context is just an String and tis is done on Updater class

This is a background thread and application is not in thread context.

martin-g · 2021-11-08T09:52:07Z

+		try {
+			this.pageClass = (Class<IManageablePage>)Thread.currentThread().getContextClassLoader().loadClass(pageClass);
+		} catch (ClassNotFoundException e) {
+			//throw new WicketRuntimeException(e);


This does not look good! Most probably we should not ignore the problem.

Yes. No longer pertinent.

martin-g · 2021-11-08T09:53:49Z

 	 * The name of the request parameter that holds the application name
 	 */
 	private static final String WICKET_APP_PARAM_NAME = "wicket-app-name";
+	private static final String WICKET_SESSION_ID = "jsessionid";


this is not used

martin-g · 2021-11-08T09:54:14Z

 	public void onOpen(Session session, EndpointConfig endpointConfig)
 	{
 		String appName = getApplicationName(session);
+		session.getId();


Hum... Got rid of this

reiern70 · 2021-11-08T11:16:04Z

@martin-g many thanks for your comments! I will go through them right now

reiern70 · 2021-11-08T11:22:03Z

About the new pageClass attribute:

PageIdKey is no more accurate name, since it is not just the id anymore. But renaming it would be an API break

What if someone wants to do the same with a resource based WebSocket behavior ?

Good point. This is why I thought about context. Maybe call this context and move this to a common Ikey?

IMO we should move this logic at IKey level. And if we can avoid class loading it would be better, but I am not sure how exactly to do it yet!

Ok. I will give it a try.

reiern70 · 2021-11-09T06:43:42Z

@martin-g I have moved context to IKey. See

74bea3b

martin-g · 2021-11-10T13:37:38Z

+import org.apache.wicket.protocol.http.WebSession;
+import org.apache.wicket.request.Request;
+
+public class JSR356Session extends WebSession {


The opening curly bracket should be on a new line.

reiern70 · 2021-11-11T08:39:21Z

@martin-g Many thanks for review and useful comments! I will squash commits and merge to wicket 9.x.

martin-g · 2021-11-11T08:42:05Z

And cherry-pick the final commit to master please!

…ages 2) pass page class as a kind of context that can be used to send a web socket to different pages + example of updating a component in a given page (not dependent on page ID)

reiern70 · 2021-11-11T10:05:18Z

And cherry-pick the final commit to master please!

Done! Thanks again for your help!

reiern70 requested a review from martin-g November 7, 2021 09:59

reiern70 mentioned this pull request Nov 7, 2021

create demo page showing how to use push in order to update a compone… #469

Closed

martin-g reviewed Nov 8, 2021

View reviewed changes

reiern70 requested a review from martin-g November 10, 2021 08:21

martin-g reviewed Nov 10, 2021

View reviewed changes

reiern70 requested a review from martin-g November 11, 2021 07:09

martin-g approved these changes Nov 11, 2021

View reviewed changes

WICKET-6930: This commit adds: 1) allow to skip sending empty WS mess…

04aaa7f

…ages 2) pass page class as a kind of context that can be used to send a web socket to different pages + example of updating a component in a given page (not dependent on page ID)

reiern70 force-pushed the WICKET-6930-websocket-improvements branch from dd443bc to 04aaa7f Compare November 11, 2021 10:00

reiern70 merged commit 04aaa7f into wicket-9.x Nov 11, 2021

solomax deleted the WICKET-6930-websocket-improvements branch April 28, 2022 06:25

Conversation

reiern70 commented Nov 7, 2021

Uh oh!

reiern70 commented Nov 7, 2021

Uh oh!

martin-g left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reiern70 commented Nov 8, 2021

Uh oh!

reiern70 commented Nov 8, 2021 • edited by martin-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reiern70 commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reiern70 commented Nov 11, 2021

Uh oh!

martin-g commented Nov 11, 2021

Uh oh!

reiern70 commented Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martin-g left a comment •

edited

Loading

reiern70 commented Nov 8, 2021 •

edited by martin-g

Loading