Sockets: Go rewrite #3536

Morfent · 2017-05-21T06:55:00Z

WIP

This turned into a pretty substantial refactor of the Node version of
sockets-related code to not only make using Go child processes
possible, but to optimize it and make unit testing of it and any code
dependent on it possible to write entirely synchronously. sockets.js
and sockets-workers.js are now written in Typescript, though they won't
be able to be transpiled until after repl.js, crashmonitor.js, Config, Users, Dnsbl, and Monitor
work with it as well.

Breaking changes:

this changes the requirements for the values of Config.ssl.options.cert and Config.ssl.options.key; they must be absolute paths to their corresponding files, not a buffer of their files' contents**

Fixes #2943

Morfent · 2017-05-27T01:36:02Z

I should probably explain the changes I made:

sockets.js was split into two files:
- sockets.js, for handling logic related to being the cluster master and spawning Go and Node workers
- sockets-workers.js for handling logic related to Node workers. I created a Multiplexer class to handle parsing downstream messages and sending them to the appropriate sockets
sockets.js has a GoWorker class that acts as a mock for Cluster.Worker to allow spawning a TCP server for IPC with the child process, spawning the child process, and begin handling IPC with the child process once the TCP server receives a connection from the child process
sockets.js has a WorkerWrapper class to wrap Node workers and GoWorker instances. This handles events common to both types of Workers, such as spawning a new worker if it crashes/gets killed and passing upstream messages to Users messages. This also allows spawning both Go and Node workers without deoptimizing Connection, User, or any other object dependent on workers to prevent methods throughout PS dependent on workers from becoming polymorphic. The only condition is Go workers must use a different port to serve with than the Node workers.
The sockets.js tests were rewritten to be completely synchronous using Multiplexer. They take a fraction of the time it took to run the old unit tests, since they were dependent on promises and hacky eval messages sent to child processes that had no need to be spawned in the first place.
pokemon-showdown verifies that Go environment variables are set, fetches dependencies if they're missing, and compiles github.com/Pokemon-Showdown/sockets/lib/, if Config.golang is set to true

On the Go side of things:

sockets/main.go sets up the socket multiplexer and IPC connection, launches the static and SockJS servers, and initializes the master/worker struct to allow concurrency in updating the state of the socket multiplexer.
sockets/lib/config.go defines the struct that represents the config settings passed from the parent proess through the $PS_CONFIG environment variable
sockets/lib/commands.go defines the struct (which is an abstraction of the format of messages sent over IPC) used by the socket multiplexer and IPC connection to pass to workers to be enqueued and passed to an available worker to process their payload and invoke methods on the socket multiplexer or IPC connection to process.
sockets/lib/ipc.go defines the struct that represents the TCP connection to the parent process. It listens for incoming messages, splits them using the DELIM character, and creates a command struct to pass to the command queue.
sockets/lib/master.go defines a master struct and a worker struct. This is an emulation of Node's cluster module, spawning a number of workers equal to Config.workers. A command queue receives command structs from the socket multiplexer and IPC connection; The master takes a worker's command queue out of its worker pool, allows said worker to receive a command from the command queue channel, then executes its payload. The master then takes the worker command queue and sends it back to the master's worker pool. tl;dr this implements a master/worker design pattern similar to how Node's cluster module handles concurency.
sockets/lib/multiplexer.go defines the struct that manages socket and worker state. *Multiplexer.Handle takes incoming sockets, stores them in *Multiplexer.sockets, then waits for incoming commands to update its state.
sockets/lib/*_test.go are unit tests for the files whose name precedes the _test.go part of the filename. Unit tests can be run using go test github.com/Zarel/Pokemon-Showdown/sockets/lib/; adding the -race flag will test for race conditions.

I had to use a rather hacky way to allow compiling the Go files. Rather than using go get github.com/Zarel/Pokemon-Showdown/sockets/, I made pokemon-showdown create src/github.com/Zarel/Pokemon-Showdown dirs in $GOPATH, then symlinked sockets/ to $GOPATH/src/github.com/Zarel/Pokemon-Showdown/sockets/ to prevent duplicating the rest of the files in the repo.

KamilaBorowska

Good job on this 👍.

KamilaBorowska · 2017-05-27T19:16:54Z

sockets/lib/commands.go

+const CHANNEL_REMOVE string = "-"
+const CHANNEL_BROADCAST string = "#"
+const SUBCHANNEL_MOVE string = "."
+const SUBCHANNEL_BROADCAST string = ":"


Pretty sure those could be byte instead of string. Also, they could be grouped like this.

const ( SOCKET_CONNECT byte = '*' SOCKET_DISCONNECT byte = '!' ... )

Originally I chose string over byte so I wouldn't have to cast command tokens to string in Command.Message, but looking over the other code dealing with these, it makes no difference which type it is. It'd make more sense for them to be byte instead

KamilaBorowska · 2017-05-27T19:21:21Z

sockets/lib/ipc.go

+// This must be a byte that stringifies to either a hexadecimal or Unicode
+// escape code. Otherwise, it would be possible for someone to send a message
+// with the delimiter and break up messages.
+const DELIM byte = '\u0003'


byte is not Unicode character, you may want to use '\x03' instead to clarify (this is allowed, because its within byte bounds, but it's not exactly clear).

Would it make more sense to make it a rune for this?

Never mind, I forgot the bufio.Reader instance requires that Reader.ReadBytes takes a byte as its delimiter

KamilaBorowska · 2017-05-27T19:23:09Z

sockets/lib/ipc.go

+		port:      port,
+		addr:      addr,
+		conn:      conn,
+		listening: false}


Probably look better if } would be on its separate line (with trailling comma after false).

I think it looks better like this, since it doesn't take up as much space

I would extremely strongly prefer it on a separate line, but I'll defer to gofmt.

gofmt has no opinion on this, and neither does Google's style guide afaik. } on a separate line it is

On the other hand, no multiline struct literal in Go style guide follows this style (singleline do, but not multiline).

(I really feel like gofmt should be able to do more than just modify whitespace, but currently it only can modify whitespace, it doesn't modify tokens in any case)

The closest example I can find is this, which does follow this style https://golang.org/doc/effective_go.html#maps

Morfent · 2017-06-10T21:56:57Z

Can someone take a look at the refactors I've made to GoWorker and WorkerWrapper to ensure the logic I use, specifically with GoWorker, is sound?

Morfent · 2017-06-10T22:03:52Z

@Zarel @xfix

KamilaBorowska · 2017-06-12T10:52:18Z

sockets/lib/commands.go

+	// themselves.
+	switch token {
+	case SOCKET_DISCONNECT:
+		count = 1


Probably can directly return instead of returning a variable. Also, some cases can be merged to avoid code duplication.

On the other hand go test complains that there's no return statement at the end of the function if I return from the switch cases. I could keep count, but still merge them so the switch isn't so redundant

Fair enough. At the same time, receiving unexpected message is a bug, and probably should be reported somehow.

I suppose it's possible if a mistake is made with /eval or if new message types are introduced.

User still should be informed about it.

KamilaBorowska · 2017-06-12T10:57:58Z

sockets/lib/multiplexer.go

+	"sync"
+	"sync/atomic"
+
+	"github.com/igm/sockjs-go/sockjs"


Should it be using development version of SockJS (as opposed to stable gopkg.in/igm/sockjs-go.v2/sockjs)?

The stable version doesn't have *session.Request,yet, which is needed to get the socket's remote address and X-Forwarded-For headers to pass back to the parent process

KamilaBorowska · 2017-06-12T10:59:49Z

sockets/lib/multiplexer.go

+	smux     sync.Mutex                // nsid and sockets mutex.
+	channels map[string]Channel        // Map of channel (i.e. room) IDs to channels.
+	cmux     sync.Mutex                // channels mutex.
+	scre     *regexp.Regexp            // Regex for splitting subchannel broadcasts into their three messages.


Does it need to be part of a struct, considering scre is a constant?

I think it fits better in the struct, since only *Multiplexer.subchannelBroadcast needs to be able to use it, unlike the other constants floating around in the file's scope

KamilaBorowska · 2017-06-12T11:02:00Z

sockets/lib/ipc.go

+		listening: false,
+	}
+
+	return


Any reason to use return syntax here, when return c, nil I think would be more clear?

At one point I was returning err instead of creating my own errors to return, so it made more sense at the time. It doesn't anymore though

KamilaBorowska · 2017-06-12T11:04:10Z

sockets/lib/master.go

+type worker struct {
+	wpool chan chan Command // The master's pool of worker command queues.
+	cmdch chan Command      // Queue for incoming commands from CmdQueue.
+	quit  chan bool         // Channel used to kill the worker when needed.


You can use close on cmdch instead of making another channel just for closing purposes.

Wouldn't that risk deadlocking if *master.Listen tries to send a command to worker.cmdch after it has closed?

KamilaBorowska · 2017-06-12T11:06:21Z

sockets/lib/commands.go

+// The multiplexer and the IPC connection both implement this interface. Its
+// purpose is solely to allow the two structs to be used in Command.
+type CommandIO interface {
+	Process(Command) (err error) // Invokes one of its methods using the command's token and parametres.


No need to name output variable name of interface, it's clear that error type is... an error.

KamilaBorowska · 2017-06-12T11:08:05Z

sockets/lib/ipc.go

+}
+
+// Final step in evaluating commands targeted at the IPC connection.
+func (c *Connection) Process(cmd Command) (err error) {


Just use unnamed error, no need to use obvious output variable names.

KamilaBorowska · 2017-06-13T08:27:01Z

pokemon-showdown

+		console.log('The GOPATH environment variable is not set! It is required in order to run the server using Go.');
+		process.exit(0);
+	}
+	if (!process.env.GOROOT) {


GOROOT is optional, and probably shouldn't be set, as its default should be fine. Don't require it, it's probably a bad idea to set that variable explicitly anyway.

I know it's a bad idea to set it to something other than /usr/local/go or whatever the equivalent on Windows is, but Go will still complain about it sometimes. Maybe I'm remembering this from using older versions though. Using go env GOROOT seems like the safer option to me just to be explicit without making users having to set $GOROOT manually

KamilaBorowska · 2017-06-13T08:31:34Z

pokemon-showdown

+} catch (e) {}
+
+if (config && config.golang) {
+	if (!process.env.GOPATH) {


GOPATH is optional in newer versions of Golang. You may want to assume $HOME/go path if it's not set or run go env GOPATH to obtain its location.

go env GOPATH sounds like the safer bet

KamilaBorowska · 2017-06-13T08:33:05Z

pokemon-showdown

+	let stat;
+	let needsSrcDir = false;
+	try {
+		stat = fs.lstatSync(path.resolve(GOPATH, 'src/github.com/Zarel'));


GOPATH is allowed to be a colon separated list (semicolon on Windows), in such an event, you may want to consider its first element. You can split the list by path.delimiter

Huh, I wasn't aware Go would allow that. When I tested it with colon separated paths, only the first one in the list got used, so doing this seems sound

KamilaBorowska · 2017-06-13T08:43:27Z

pokemon-showdown

+	if (!stat || !stat.isSymbolicLink()) {
+		try {
+			if (process.platform === 'win32') {
+				child_process.execSync(`mklink /J ${tarPath} ${srcPath}`, {stdio: 'inherit'});


fs.symlinkSync

First time I tried that for Windows symlinks it required admin access, but I didn't realize junctions didn't at the time, which is dumb since I'm already using junctions here.

KamilaBorowska · 2017-06-13T08:43:44Z

pokemon-showdown

+			if (process.platform === 'win32') {
+				child_process.execSync(`mklink /J ${tarPath} ${srcPath}`, {stdio: 'inherit'});
+			} else {
+				child_process.execSync(`ln -sF ${srcPath} ${tarPath}`, {stdio: 'inherit'});


Not sure why are you using -F, it's pointless here, as this isn't a hard link.

KamilaBorowska · 2017-06-13T08:47:31Z

Also, forgot to mention that node app, original way to start Pokemon Showdown (as opposed to npm start or ./pokemon-showdown) without dependencies already installed quietly fails (gets stuck) for some reason. Probably should just error out.

Morfent · 2017-06-13T08:57:11Z

It doesn't kill the process for you when you try that? It does when I try it. It already throws an error when that happens, but I can change it to log the error to console and exit the process instead.

Morfent · 2017-06-13T08:59:19Z

It also exits if config/config.jsdoesn't exist, even after having created it. I'll see if I can fix that as well,
though that'd be more appropriate to make as a pullreq apart from this one

Morfent · 2017-08-11T13:17:37Z

It's been a month since my last commit, but I honestly can't think of any glaring issues left to fix. Is there some way to give this a test run on a reasonably active server? I still think it's too soon to be sure that this won't break on main, but any types of bugs left to find are not going to be the type I can find screwing around with a server with a dozen users.

Zarel · 2017-08-12T04:55:20Z

Sure, rebase it and I'll test it on Smogtours.

Morfent · 2017-08-12T06:05:36Z

Rebased

Morfent · 2017-09-17T17:48:07Z

...not sure why I commented and never got around to it. Now it actually is though

Zarel · 2017-09-17T23:51:46Z

Um, we should probably deal with the other sockets pullreq first.

Morfent · 2017-09-19T17:01:30Z

Rebased

Zarel · 2017-09-19T17:03:21Z

Apparently still conflicting?

Zarel · 2017-09-19T17:04:01Z

Also: is it possible for you to pullreq into a new branch instead of directly into master?

Morfent · 2017-09-19T17:09:10Z

Alright

Fixes smogon#2943

Slayer95 · 2019-08-15T12:49:39Z

[07:45] ~pre: migration to go was cancelled

Morfent force-pushed the sockets branch 3 times, most recently from 912a397 to 8837727 Compare May 21, 2017 07:15

Morfent force-pushed the sockets branch from e2c5974 to 0b5d986 Compare May 27, 2017 18:54

KamilaBorowska reviewed May 27, 2017

View reviewed changes

Morfent force-pushed the sockets branch 5 times, most recently from ba5a655 to 1f84f59 Compare May 28, 2017 02:44

Morfent force-pushed the sockets branch 3 times, most recently from 4cd84e4 to 2b71e64 Compare June 7, 2017 08:35

Morfent force-pushed the sockets branch 7 times, most recently from 9f7b077 to 1ab65e3 Compare June 12, 2017 02:12

KamilaBorowska requested changes Jun 12, 2017

View reviewed changes

Morfent force-pushed the sockets branch from 9497e7e to 4876805 Compare June 12, 2017 18:43

KamilaBorowska requested changes Jun 13, 2017

View reviewed changes

Morfent mentioned this pull request Jun 13, 2017

App: create default config file in launch script #3630

Merged

Morfent force-pushed the sockets branch 4 times, most recently from 52954fd to 2ddba56 Compare July 5, 2017 22:46

Morfent force-pushed the sockets branch from 2ddba56 to 6e3e4dd Compare July 13, 2017 13:43

Slayer95 force-pushed the master branch from 1aabf2d to f02b22e Compare August 7, 2017 14:37

Morfent force-pushed the sockets branch from 6e3e4dd to c9e20d7 Compare August 12, 2017 05:58

Morfent force-pushed the sockets branch 4 times, most recently from 92aab9d to d0110b3 Compare September 17, 2017 17:33

Morfent force-pushed the sockets branch 2 times, most recently from 9445c21 to ddb4153 Compare September 17, 2017 18:12

Morfent force-pushed the sockets branch from af3850d to c209d60 Compare September 19, 2017 17:01

Morfent changed the base branch from master to sockets-go September 19, 2017 17:08

Morfent force-pushed the sockets branch from c209d60 to b2eb121 Compare September 19, 2017 17:08

Morfent force-pushed the sockets branch from b2eb121 to 6a95158 Compare September 19, 2017 17:28

Sockets: Go rewrite

131bf27

Fixes smogon#2943

Morfent force-pushed the sockets branch from 6a95158 to 131bf27 Compare November 6, 2017 06:18

Slayer95 closed this Aug 15, 2019

Sockets: Go rewrite #3536

Sockets: Go rewrite #3536

Conversation

Morfent commented May 21, 2017 • edited

Morfent commented May 27, 2017 • edited

KamilaBorowska left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KamilaBorowska May 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morfent May 27, 2017 • edited

Choose a reason for hiding this comment

KamilaBorowska May 27, 2017 • edited

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

Morfent commented Jun 10, 2017

Morfent commented Jun 10, 2017

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

Choose a reason for hiding this comment

Morfent Jun 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morfent Jun 12, 2017 • edited

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

Morfent Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KamilaBorowska Jun 13, 2017 • edited

Choose a reason for hiding this comment

Morfent Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morfent Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KamilaBorowska commented Jun 13, 2017 • edited

Morfent commented Jun 13, 2017 • edited

Morfent commented Jun 13, 2017 • edited

Morfent commented Aug 11, 2017

Zarel commented Aug 12, 2017

Morfent commented Aug 12, 2017

Morfent commented Sep 17, 2017

Zarel commented Sep 17, 2017

Morfent commented Sep 19, 2017

Zarel commented Sep 19, 2017

Zarel commented Sep 19, 2017

Morfent commented Sep 19, 2017

Slayer95 commented Aug 15, 2019

Morfent commented May 21, 2017 •

edited

Morfent commented May 27, 2017 •

edited

KamilaBorowska left a comment •

edited

KamilaBorowska May 27, 2017 •

edited

Morfent May 27, 2017 •

edited

KamilaBorowska May 27, 2017 •

edited

Morfent Jun 12, 2017 •

edited

Morfent Jun 12, 2017 •

edited

Morfent Jun 13, 2017 •

edited

KamilaBorowska Jun 13, 2017 •

edited

Morfent Jun 13, 2017 •

edited

Morfent Jun 13, 2017 •

edited

KamilaBorowska commented Jun 13, 2017 •

edited

Morfent commented Jun 13, 2017 •

edited

Morfent commented Jun 13, 2017 •

edited