You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by GitBox <gi...@apache.org> on 2020/03/20 15:40:28 UTC
[GitHub] [wicket] theigl opened a new pull request #417: WICKET-6761
Multiple websocket connections from the same session
theigl opened a new pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417
This PR adds an optional `resourceToken` to `BaseWebSocketBehavior` to support multiple websocket connections from the same session, i.e. from multiple browser tabs.
The token could be a UUID or some other calculated value.
Note: I'm not really happy about the name `resourceToken`. Maybe something like `connectionId` would be easier to understand.
See https://issues.apache.org/jira/browse/WICKET-6761
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r395963983
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
Why the token is supported only for Resource but not for Page ?
In case of a Page there will be locking on the page instance and it will work sequential but it should still work, I think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r395979705
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
@martin-g: My reasoning was this: Pages already work with multiple tabs because the connection key contains the `pageId` with is unique it to each tab as far as I understand. We could pass it for pages as well and add it to the key but it wouldn't really improve things.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r396433237
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
I can add the `connectionToken` to pages as well if we want to go ahead with this approach. For now, I kept it just for resources.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r396431582
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
Martin: You are right! I did not test this PR yet, it was meant as a basis for discussion and to clarify what I'm trying to achieve.
I now added a demo page to the websocket examples at: http://localhost:8080/websockets
It uses `UUID.randomUUID()` as a token. If you open two tabs of the new page, you will see that the charts are updated correctly. If you open two tabs of the existing demo page, the first tab will stop receiving updates because the connection has been overwritten.
I had to make two changes to the existing demo to showcase this:
- Increase the pool size of the scheduled executor to support multiple tabs
- Move connection lookup in `ChartUpdater` into the loop so the connection is not cached but looked up every time the chart wants to push new data. Even though the connection was overwritten in the registry, the demo still worked before because `ChartUpdater` held a reference to the connection.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r396389423
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
`connectionToken` sounds better.
But who is creating this token ? I didn't see anything about this in your PR.
Have you tested your PR? Does it really solve your issue ? Because due to the typo in the Javascript code I have the feeling you didn't actually used this code.
Please add changes to the Wicket Examples demo application that makes it easier for me to play with this new functionality.
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r396376097
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
> contains the `pageId` with is unique it to each tab
This is not correct!
If the user copies the url from the browser's address bar and pastes it in a new tab then the page id is the same.
This is why we have https://github.com/apache/wicket/blob/master/wicket-core/src/main/java/org/apache/wicket/ajax/AjaxNewWindowNotifyingBehavior.java
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r396380385
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/BaseWebSocketBehavior.java
##########
@@ -93,10 +119,12 @@ public void renderHead(Component component, IHeaderResponse response)
int pageId = component.getPage().getPageId();
variables.put("pageId", pageId);
variables.put("resourceName", "");
+ variables.put("resourceToken", "");
Review comment:
Ah ok. I have been using wicket with "pretty" URLs without any `pageId` parameters for so long that I forgot how it works out of the box.
If we want to use it for pages as well, we definitely need a better name for the parameter. What do you think about `connectionId` or `connectionToken`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g merged pull request #417: WICKET-6761 Multiple
websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r395979587
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/res/js/wicket-websocket-jquery.js
##########
@@ -76,6 +76,9 @@
url += '?pageId=' + encodeURIComponent(WWS.pageId);
} else if (WWS.resourceName) {
url += '?resourceName=' + encodeURIComponent(WWS.resourceName);
+ if (WWS.resourceToken) {
+ url += '&resourceToken=' + encodeURIComponent(WWS.resourceName);
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on issue #417: WICKET-6761 Multiple
websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
martin-g commented on issue #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#issuecomment-606453383
Thank you, @theigl !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on a change in pull request #417:
WICKET-6761 Multiple websocket connections from the same session
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #417: WICKET-6761 Multiple websocket connections from the same session
URL: https://github.com/apache/wicket/pull/417#discussion_r395963529
##########
File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/res/js/wicket-websocket-jquery.js
##########
@@ -76,6 +76,9 @@
url += '?pageId=' + encodeURIComponent(WWS.pageId);
} else if (WWS.resourceName) {
url += '?resourceName=' + encodeURIComponent(WWS.resourceName);
+ if (WWS.resourceToken) {
+ url += '&resourceToken=' + encodeURIComponent(WWS.resourceName);
Review comment:
encodeURIComponent(WWS.resource**Token**);
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services