Skip to content
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

Websockets #1322

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Websockets #1322

wants to merge 13 commits into from

Conversation

jbrichau
Copy link
Member

Added support for Websockets in Seaside.

@jbrichau jbrichau marked this pull request as draft August 16, 2022 14:22
@@ -0,0 +1,8 @@
printing
javascriptContentOn: aStream
"TODO: window.location should not be retrieved here..."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use #baseUrl or something similar here instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. It should probably use the settings of WARequestHandlingConfiguration... it's just that I do not prefer that way of working. Alas, I see no alternative either.

@@ -0,0 +1 @@
I am a WebSocket.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More an abstract class with server specific implementations.

protected
websocketEventHandlerJavaScriptOn: aStream
aStream
nextPutAll: '.addEventListener(''message'', (event) => { Function(`"use strict";return (${event.data})`)(); })'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the need for a function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sockets
"Answer the current list of sockets."

^ sockets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this error prone because the caller has to care about the mutex? Shouldn't we just offer #socketsDo:?

@@ -0,0 +1,4 @@
protected
socketsDo: aOneArgumentBlock
self mutex critical: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a ^ be helpful here so the sender can easily access the return value from the provided block?

@@ -1,6 +1,9 @@
converting
responseFrom: aRequestContext
| partialHeaders cookies fullHeaders seasideResponse contents entity contentType |
"TODO: is websocket only... useful/needed to package in separate package?"
(aRequestContext properties at: #webSocket ifAbsent:[ nil ])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the formatting in a lot of places omits a space after :. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a lot to clean up still and there’s a lot of copy paste for now. I made a draft PR to follow up on the CI build. So most of this code is not in the final clean state, rather fast prototyping.

Transcript cr; show: 'New ', aWAWebSocket printString.

aWAWebSocket onMessage:[ :data |
self socketsDo: [ :socket | socket send: data ] ].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the socket iteration pattern.

  • In this case, and probably others, since #socketsDo: acquires a mutex it blocks until data has been sent to all sockets. I wonder if it would be beneficial to have some sort of job/task queue and submitting a task for every socket and hold the mutex only for submitting the tasks.
  • Between when we decided to send data to the sockets and actually doing it the socket may have become closed or invalid which makes we wonder whether we should:
    • check each web socket for validity in #send: and possibly removing it
    • wrap each #send: in an #on:do:
  • Having all the sockets in an array, linearly scanning over them and hoping #onClose: will always get sent and the web socket removed. I'm concerned with scalability and that we end up with dead or closed web sockets in the array that never get removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, all valid concerns and this still needs to be addressed. In addition, the seaside session needs to be kept alive as long as the sockets are communicating/existing. While doing experiments with the simple counter example, I encounter websocket connections being closed without any callback firing without a consistent timeout or other immediately visible reason. So, I still have a long way to go to make sure this works in practice.

@@ -1,6 +1,9 @@
converting
responseFrom: aRequestContext
| partialHeaders cookies fullHeaders seasideResponse contents entity contentType |
"TODO: is websocket only... useful/needed to package in separate package?"
(aRequestContext properties at: #webSocket ifAbsent:[ nil ])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do something special when shutting down the server?

@@ -0,0 +1,3 @@
accessing
addSocket: aWebSocket
mutex critical: [ sockets := sockets copyWith: aWebSocket ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention of using CoW here that readonly access to the sockets (eg. just iterating) can be done without a mutex?

# Conflicts:
#	repository/BaselineOfSeaside3.package/BaselineOfSeaside3.class/instance/baselineadaptors..st
#	repository/Seaside-Zinc-Core.package/ZnZincServerAdaptor.class/instance/websocketResponseFrom..st
Copy link

codecov bot commented Jul 14, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 48.95%. Comparing base (cb4b8ef) to head (944f3e1).
Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
...erAdaptor.class/instance/websocketResponseFrom..st 0.00% 9 Missing ⚠️
...ZnZincServerAdaptor.class/instance/statusString.st 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1322      +/-   ##
==========================================
- Coverage   49.04%   48.95%   -0.09%     
==========================================
  Files        9143     9160      +17     
  Lines       80555    80910     +355     
==========================================
+ Hits        39506    39610     +104     
- Misses      41049    41300     +251     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants