Skip to content

Conversation

@JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Mar 18, 2025

Pull Request Description

Resolves #11477 by making callbacks from ydoc-polyfill WebSocket emulation via the same ScheduledExecutorService as provided by EnsoLanguage. In addition to that it registers threadAccessDeniedHandler to coordinate JavaScript multi threaded execution.

Important Notes

The EpbLanguage (which injects the WebSocket emulation) locates the EnsoLanguage by its ID and obtains a reference to EnsoContext.getThreadManager() via lookup(langInfo, ScheduledExecutorService.class). Then it uses the obtained executor service for making callbacks into JavaScript whenever a websocket event arrives.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 18, 2025
@JaroslavTulach JaroslavTulach self-assigned this Mar 18, 2025
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Mar 18, 2025

Manual Testing

Let's use a simple script that connects to echo service at http://websocket.org to verify how well the support for WebSocket API works in Enso IDE:

var counter = 0;
function app() {
    print("Connecting to websocket.org...");
    let ws  = new WebSocket('wss://echo.websocket.org');
    function sayHi() {
      ws.send(`Hi there! #${counter++}`);
    }
    ws.onopen = function(ev) {
      print("Open: " + ev);
    };
    ws.onmessage = function(ev) {
      print("Msg   : " + Object.getOwnPropertyNames(ev));
      print("  data: " + ev.data);
      setTimeout(sayHi, 3000);
    };
    ws.onclose = function(ev) {
      print("Close: " + Object.getOwnPropertyNames(ev));
    };
    ws.onerror = function(ev) {
      print("Err: " + Object.getOwnPropertyNames(ev));
    };
}

if (typeof insight === "undefined") {
    globalThis.print = console.log;
    if (typeof WebSocket === "undefined") {
        let r = require("ws");
        globalThis.WebSocket = r.WebSocket;
    }
    app();
} else {
    app();
}

The script is generic. You can use it in node.js as well as Enso IDE or command line interface.

Enso GUI

Store the above script as /tmp/wstest.js and then execute following commands in the root of Enso repository:

enso$ export ENSO_JVM_OPTS="-Denso.dev.insight=/tmp/wstest.js -Dpolyglot.enso.interpreter.jobParallelism=1"
enso$ sbt runProjectManagerDistribution
....
Connecting to websocket.org...
...
Msg   : 
  data: Hi there! #2
...

and in another terminal execute corepack pnpm dev:gui. Connect your browser to http://localhost:5173/, open a project and observe messages printed by the sbt runProjectManagerDistribution. The same output as in case of node.js (see below) shall appear.

Verify with node.js

Save the above script into an empty directory ws as wstest.js and execute:

ws$ npm install ws
ws$ node wstest.js 
Connecting to websocket.org...
Open: [object Event]
Msg   : 
  data: Request served by 1781505b56ee58
Msg   : 
  data: Hi there! #0
Msg   : 
  data: Hi there! #1
Msg   : 
  data: Hi there! #2

and so on. Every few seconds another "Hi there!" message with an increased counter shall appear. The same output shall be visible in the Enso GUI as well as from CLI.

@JaroslavTulach JaroslavTulach requested a review from Frizi March 18, 2025 11:16
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Mar 18, 2025

@hubertp, @4e6, do you think following assumption:

the assumption that all Enso code is executing in the pool controlled by the jobParallelism property

Is supposed to be true? E.g. if I fix the violations and set jobParallelism=1 - will JavaScript callbacks be properly serialized one and another and intermixed with regular requests to execute Enso code?

Update on Apr 10, 2025:

@JaroslavTulach JaroslavTulach changed the title Share worker thread among RuntimeServerInstrument and ydoc-polyfill Share getThreadManager pool among RuntimeServerInstrument and Websocket emulation Apr 10, 2025
@JaroslavTulach
Copy link
Member Author

We seem to have problems with shutdown:

The language did not complete all polyglot threads but should have: [Thread[#546,guest-code-1,5,main]]

probably the shutdown isn't done properly.

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/ShareWorkerThread11477 branch from c9e4fbe to 4cc47eb Compare April 10, 2025 06:24
}
: null;
var allDefs = localScope.allSymbols(name, logFnOrNull);
var allDefs = localScope.allSymbols(name, null);
Copy link
Member Author

@JaroslavTulach JaroslavTulach Apr 11, 2025

Choose a reason for hiding this comment

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

b5b430a avoids usage of TruffleLogger from non-guest code threads.

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

OK


final void handleMultiAccess(String msg) {
try {
var ms = delayer.nextInt(10, 1000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we log this event always?

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 was thinking about some heuristics to decide when to log, but
  • at the end I decided to leave it out of this PR
  • I am not sure logging (and dumping all the thread stacks) always is what we want
  • there might be plenty of events like this
  • a one line note in the log when verbosity is high might be OK
  • so far I haven't seen many clashes even I am running with -ea on

@JaroslavTulach
Copy link
Member Author

@JaroslavTulach JaroslavTulach merged commit 4f0f5f6 into develop Apr 11, 2025
72 of 73 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/ShareWorkerThread11477 branch April 11, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: No changelog needed Do not require a changelog entry for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow to execute Y.js and Insight together

2 participants