Use socket for SC -> JS communication #37

ssfrr · 2017-09-27T07:08:35Z

This PR still needs some testing but seems functional. The two main parts are:

Now uses a separate socket for getting sending back the result of an
interpret call from SCLang. This way you can rely on getting the data
back intact even if there's other stuff going over STDOUT. For example, if
you're printing OSC debugging info while at the same time requesting data
to use for auto-complete in the editor.
Reworks the STDOUT handling state machine to work line-by-line. I wasn't
planning on doing this as part of this PR but I ran into trouble where
sometimes the state machine wasn't grabbing the CAPTURE:END blocks. IMO
this setup is easier to read and no longer makes any assumptions about
alignment of the callback data.

I still need to walk through the state machine to verify it's correctly
handling all the cases that the old one did before this is ready for full
review, but I figured I'd post progress in case you wanted to take a glance.

crucialfelix · 2017-09-27T07:54:57Z

You will want to use the test suite. Once you turn on jest it runs those tests before you can even blink.

I'm not sure what the address in use error is in here: https://travis-ci.org/crucialfelix/supercolliderjs/jobs/280304528

crucialfelix · 2017-09-27T08:11:16Z

Looks great so far !

ssfrr · 2017-09-28T09:15:23Z

OK, it's passing all the tests. I've done a little refactoring (I moved the socket stuff into sclang.js from sclang-io.js, and made sure the connection is cleaned up on quit. I also confirmed that this fixes #34.

Also closes #35.

I've tested it a bit running from node directly and also playing around in Atom, and AFAICT it seems to be working fine. Definitely kick the tires and let me know if there are issues.

crucialfelix · 2017-09-28T19:34:50Z

I will definitely kick tires and do code review. Great work !

crucialfelix · 2017-10-07T08:46:32Z

I haven't yet had a chance to test this. I will probably want to merge it to develop, but then there are some issues that need to be solved before a release.

Currently you could only run one supercollider.js at the same time. I often run many.

So the socket should be able to use unix domain sockets like /var/run/supercolliderjs_{PID} (id of the node.js process) and pass that through to supercollider. That would guarantee a unique, secure and easily configurable channel of communication.

But we should also support open TCP connections. It would be fun (and possibly not entirely useless) to connect to an sclang running on a beagleboard.

crucialfelix · 2017-09-27T08:10:48Z

src/lang/internals/sclang-io.js

-  READY: 'ready'
+  CONNECTING: 'connecting',
+  READY: 'ready',
+  CAPTURING: 'capturing',


I think you want to keep capturing state in a separate variable. These states are broadcast to listeners mainly for watching when sclang is up and ready (or died etc.) You wouldn't want to broadcast a change any time there is an incoming response.

crucialfelix

Well, the years go by quick. I do apologize. I have looked at it a few times, but kids, jobs and other ambitions are pretty distracting.

The main goal of the PR (as I understand it) is to allow larger responses to be returned from sclang without breaking or getting co-mingled with other STDOUT that sclang may be burping up.

The main blocker to landing this is that it doesn't just add a TCP connection for returning responses. It also reworks the state machine and parsing. It adds more places for things to go wrong or for internal state to get out of sync.

It's a bit messier than the previous implementation and doesn't fix or change anything.

The main win here is the TCP socket for responses.

So I'd like to include just that, make it optional, configurable port and leave the rest of the state and IO parser as is. It would still be able to parse STDOUT results. The TCP version if enabled would allow results to be sent that way. Probably it could send the Errors back too. They can get quite big.

I know you've moved on to other exciting things in life, so I'm not asking for changes.

crucialfelix · 2019-06-22T12:12:00Z

src/lang/sclang.js

@@ -24,6 +25,9 @@ import { SclangResultType } from '../Types';
 // 'any' just opts out of type checking
 type ChildProcessType = any; // child_process$ChildProcess;

+const RESULT_LISTEN_HOST = 'localhost';
+const RESULT_LISTEN_PORT = 22967;


TCP port is hardcoded, and so there could only be one sclang running at a time.

crucialfelix · 2019-06-22T12:16:33Z

src/lang/sclang.js

      stateWatcher.on(name, (...args) => {
        this.emit(name, ...args);
      });
    }
+    stateWatcher.on('state', newState => {


I don't think that the stateWatcher event listener should be keeping track of changes. It is only supposed to re-emit the event. The consumer can keep track and ignore if it needs to.

The reason you had to put this in is because you added "CAPTURING" as a new state, but the intention of the states is only to track the major lifecycle of the sclang interpreter.

As you have it now it would go back and forth from READY to CAPTURING.

I think you are assuming that there can only be one interpreter call active at a time. In fact, you can launch many of them and each one many do async things over in supercollider lang, and return results or errors at any point in the future.

crucialfelix · 2019-06-22T12:19:53Z

src/supercollider-js/SuperColliderJS.sc

 		compiled = code.compile;

 		if(compiled.isNil, {
-			"\nSUPERCOLLIDERJS:%:CAPTURE:END".format(guid).postln;
+			"SUPERCOLLIDERJS:%:CAPTURE:END".format(guid).postln;


The newlines are actually necessary in case sclang posted, not postln. The SUPERCOLLIDERJS messages need to always start on a newline.

crucialfelix · 2019-06-22T12:21:27Z

src/supercollider-js/SuperColliderJS.sc

+		if(resultSocket.isNil, {
+			"resultSocket must be connected before returning data".error;
+		}, {
+			var msg = (


I think you could have just done this.stringify(msg). Looks like you tried to do that but () is an Event in sc, not a simple dict.

crucialfelix · 2019-06-22T12:22:47Z

src/lang/internals/sclang-io.js

@@ -15,9 +15,12 @@ export const STATES = {
  COMPILED: 'compiled',
  COMPILING: 'compiling',
  COMPILE_ERROR: 'compileError',
-  READY: 'ready'
+  READY: 'ready',
+  CAPTURING: 'capturing'


It adds a new STATES "CAPTURING", but the intention of the states is only to track the major lifecycle of the sclang interpreter. As you have it now it would go back and forth from READY to CAPTURING.

I think you are assuming that there can only be one interpreter call active at a time. In fact, you can launch many of them and each one many do async things over in supercollider lang, and return results or errors at any point in the future.

crucialfelix · 2019-06-22T12:24:26Z

src/lang/internals/sclang-io.js

@@ -33,70 +36,83 @@ export const STATES = {
 */
 export class SclangIO extends EventEmitter {
  states: Object;
-  responseCollectors: Object;
-  capturing: Object;


responseCollectors and capturing is a dict storing responses. There may be more than one active at any time. They can post overlapping messages to STDOUT.

crucialfelix · 2019-06-22T12:28:36Z

src/lang/internals/sclang-io.js

-          if (!stf.re.global) {
-            break;
-          }
+    var inputLines = input.split('\n');


I can see that parsing line by line is possibly better than multiline matching. I can also see a cleaner way to do it (than either my previous implementation or the one in this PR). But by changing this as much as you did, I'm now confused and can't quite tell what might break.

Yes, the tests run, but the reason I hadn't merged before is because I can't be sure now.

ssfrr · 2019-06-24T14:54:53Z

Thanks for taking the time to look at this, and sorry the PR co-mingled too many different things.

A bit after working on this PR I switched back to using the main SC IDE, so unfortunately I don't think I'll have the time to work on this any time in the near future. I won't be offended if you want to close the PR, or rip out any bits that might be useful if you still want to switch to TCP transport.

crucialfelix · 2019-06-24T15:09:48Z

I'm definitely going to transplant the TCP transport and make that work. I will try to cherry pick it in if I can, but definitely give you the credit. Thanks!

ssfrr added 5 commits September 26, 2017 19:45

working sometimes, sometimes doesn't end capture

0451c4f

seems to be working pretty reliably, need to clean up printing

313602e

cleans up debug printing, seems to be working pretty well

20b135f

synchronizes class definition fields with reset()

525607d

removes unused write function

4787a39

ssfrr added 4 commits September 27, 2017 22:49

adds SCLang.close() method to clean up connections

ff7ff17

moves socket handling from sclang-io to sclang

1a78070

fixes linter errors

c0a15c9

fixes boot return value, quiets some state transitions

b9ca930

ssfrr changed the title ~~[WIP] Use socket for SC -> JS communication~~ Use socket for SC -> JS communication Sep 28, 2017

crucialfelix mentioned this pull request Oct 30, 2017

sclang interpret string size limit #33

Open

crucialfelix approved these changes Jun 22, 2019

View reviewed changes

crucialfelix reviewed Jun 22, 2019

View reviewed changes

crucialfelix self-requested a review June 22, 2019 12:38

crucialfelix mentioned this pull request Jun 22, 2019

Communicate with sclang via TCP #45

Open

crucialfelix closed this Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use socket for SC -> JS communication #37

Use socket for SC -> JS communication #37

ssfrr commented Sep 27, 2017

crucialfelix commented Sep 27, 2017

crucialfelix commented Sep 27, 2017

ssfrr commented Sep 28, 2017

crucialfelix commented Sep 28, 2017

crucialfelix commented Oct 7, 2017 •

edited

Loading

crucialfelix Sep 27, 2017

crucialfelix left a comment

crucialfelix Jun 22, 2019

crucialfelix Jun 22, 2019

crucialfelix Jun 22, 2019

crucialfelix Jun 22, 2019

crucialfelix Jun 22, 2019

crucialfelix Jun 22, 2019

crucialfelix Jun 22, 2019

ssfrr commented Jun 24, 2019

crucialfelix commented Jun 24, 2019

Use socket for SC -> JS communication #37

Use socket for SC -> JS communication #37

Conversation

ssfrr commented Sep 27, 2017

crucialfelix commented Sep 27, 2017

crucialfelix commented Sep 27, 2017

ssfrr commented Sep 28, 2017

crucialfelix commented Sep 28, 2017

crucialfelix commented Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

crucialfelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssfrr commented Jun 24, 2019

crucialfelix commented Jun 24, 2019

crucialfelix commented Oct 7, 2017 •

edited

Loading