Switch from node.js domains to child process #258

silverbucket · 2020-03-30T18:00:11Z

Due to issues with some modules when used within a node.js domain, I've switched to running the platform activity in it's own child process. This creates an extra layer of abstraction, child process rpc communication, but overall gives us more security in the safety of the main sockethub thread, while the child processes can be destroyed and restarted whenever we get an uncaught error, or any unexpected behavior.

New child process for each unique process (a non-persistent platform like feeds which one be one generic child process anyone can use, or a persistent platform like xmpp or irc which would have a new child process for each user[actor])
Introduction of PlatformInstance class, of which each instance contains references and relevant data to the child process.
A platform.js file which is executed for each child process and is responsible for initializing the module and interfacing between it, the job queue, and the parent sockethub process.
Lots of code cleanup and improved typing

Some outstanding things to resolve:

Handle platform.js updateCredential messages (when username or password are changed from within a sockethub session, those new credentials need to be updated for that sessions verification process.
non-persistent threads (eg. feeds) need to verify they are started up correctly and managed efficiently. Some work still needs to be done here.
When the main sockethub thread throws an exception and dies, the children will still be running. Not sure if there's a way to ensure the children die, but needs to be investigated a bit more.

Some other things of note:

This actually fixes another issue where a crashed platform would disconnect the client socket. Additionally, the handling of any failure outside of the context of a job (AS command sent from client) are also reported back to the client as part of the cleanup of that thread. This means that in some cases, the client can receive an error message twice.

A job failure that also causes an uncaught exception will send a message back about the job completion, and the uncaught exception.
An uncaught exception that does not happen during a job execution will send a message back about the uncaught exception.

In neither case is the client socket disconnected, and the next time a command is sent to that platform a new process will be spawned if it doesn't already exist.

… instance of the platform it was initialized to manage

…mpleted

…ssage sent to client without disconnecting the socket.

- sockethub-platform-xmpp@3.2.2-alpha.0 - sockethub@3.2.0-alpha.0

silverbucket · 2020-03-30T18:04:34Z

published to npm: sockethub@3.2.0-alpha.0

raucao · 2020-03-30T18:55:36Z

First try in Hyperchannel: Everything works now! 🎉

raucao · 2020-03-30T19:15:43Z

Just to document it here: memory usage of the XMPP platform worker process is ~14MB on my machine just now.

So that would be about 72 platform instances per GB of memory (not counting inevitable size increase from adding platform features). Depending on how many platforms we want to spin up per Kosmos Chat user, this can quickly add up, of course. But memory is also cheap nowadays. You can get 64GB machines at Hetzner for about 40 EUR/month for example. So I wouldn't worry about this at all for our own infra. But you would obviously hit limits on cheapo cloud VMs much sooner (for people self-hosting for their family or company e.g.).

raucao

LGTM. Nice implementation! Just added a couple of nitpicks.

raucao · 2020-03-30T19:18:28Z

packages/sockethub/src/worker.ts

 import randToken from 'rand-token';
 import Store from 'secure-store-redis';
 import hash from 'object-hash';
+const { fork } = require('child_process');


Why not use an import here, too?

raucao · 2020-03-30T19:19:05Z

packages/sockethub/src/worker.ts


 import crypto from './crypto';
 import services from './services';
 import SharedResources from './shared-resources';
 import config from './config';
+import {platform} from "os";


Let's always use spaces between curly braces. Should probably be enforced by ESLint at some point soon.

raucao · 2020-03-30T19:30:30Z

Actually, after reloading Hyperchannel, I wasn't able to connect at all anymore. And it looks like the platform instance isn't terminated, even though the log says so. I can still see the process running, after Sockethub has reported that it terminated it.

Edit: I killed the process manually then, and the next connection worked fine again.
Edit 2: Just to confirm, it is reproducible. Every reload of the Web client will cause this to happen.

silverbucket · 2020-03-30T23:28:28Z

Actually, after reloading Hyperchannel, I wasn't able to connect at all anymore. And it looks like the platform instance isn't terminated, even though the log says so. I can still see the process running, after Sockethub has reported that it terminated it.

Edit: I killed the process manually then, and the next connection worked fine again.
Edit 2: Just to confirm, it is reproducible. Every reload of the Web client will cause this to happen.

Yeah I'm seeing it as well. This sent me down a path toward considering some modifications to the worker as it might be adding unnecessary complexity after the refactor to processes. I'm not totally sure yet, need to think about it some more.

…ocess manager and sends jobs to a special id to route to the platform thread

…he queue for jobs

…crypting jobs

raucao · 2020-05-05T08:08:23Z

Could you maybe post an update on what the recent commits mean? Would be cool if someone could help out here...

silverbucket · 2020-06-15T17:12:33Z

@galfert thanks for catching these, they are fixed.

galfert · 2020-06-15T20:11:32Z

Cool, the second error is gone now. But I'm still seeing the invalid property: "sessionSecret" warning for a couple of other event messages. E.g. for completed events where the @type is either join or observe.

Also, at least for XMPP, when the connection fails (e.g. because of invalid credentials), the whole Sockethub server crashes:

UNCAUGHT EXCEPTION IN PLATFORM

Error: failed connecting galfert@kosmos.org/hyperchannel : XMPP authentication failure
    at EventEmitter.error (/sockethub/packages/sockethub-platform-xmpp/index.js:556:15)
    at EventEmitter.emit (events.js:210:5)
    at Client.<anonymous> (/sockethub/packages/sockethub-platform-xmpp/node_modules/simple-xmpp/lib/simple-xmpp.js:477:20)
    at Client.emit (events.js:210:5)
    at Client._handleAuthState (/sockethub/packages/sockethub-platform-xmpp/node_modules/node-xmpp-client/lib/Client.js:295:10)
    at Client._handleStanza (/sockethub/packages/sockethub-platform-xmpp/node_modules/node-xmpp-client/lib/Client.js:233:12)
    at Client.onStanza (/sockethub/packages/sockethub-platform-xmpp/node_modules/node-xmpp-client/lib/Client.js:221:8)
    at Connection.emit (events.js:210:5)
    at Connection.onStanza (/sockethub/packages/sockethub-platform-xmpp/node_modules/node-xmpp-core/lib/Connection.js:377:10)
    at StreamParser.<anonymous> (/sockethub/packages/sockethub-platform-xmpp/node_modules/node-xmpp-core/lib/Connection.js:231:10)
    at StreamParser.emit (events.js:210:5)
    at SaxLtx.<anonymous> (/sockethub/packages/sockethub-platform-xmpp/node_modules/@xmpp/streamparser/index.js:69:14)
    at SaxLtx.emit (events.js:210:5)
    at SaxLtx._handleTagOpening (/sockethub/packages/sockethub-platform-xmpp/node_modules/ltx/lib/parsers/ltx.js:40:12)
    at SaxLtx.write (/sockethub/packages/sockethub-platform-xmpp/node_modules/ltx/lib/parsers/ltx.js:159:18)
    at StreamParser.write (/sockethub/packages/sockethub-platform-xmpp/node_modules/@xmpp/streamparser/index.js:134:17)
    at Connection.onData (/sockethub/packages/sockethub-platform-xmpp/node_modules/node-xmpp-core/lib/Connection.js:310:17)
    at TLSSocket.emit (events.js:215:7)
    at addChunk (_stream_readable.js:309:12)
    at readableAddChunk (_stream_readable.js:290:11)
    at TLSSocket.Readable.push (_stream_readable.js:224:10)
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:182:23)

UNCAUGHT EXCEPTION

TypeError: Cannot create property 'context' on string 'Error: failed connecting galfert@kosmos.org/hyperchannel : XMPP authentication failure'
    at PlatformInstance.sendToClient (/sockethub/packages/sockethub/dist/platform-instance.js:39:25)
    at ChildProcess.message (/sockethub/packages/sockethub/dist/platform-instance.js:78:26)
    at ChildProcess.emit (events.js:210:5)
    at emit (internal/child_process.js:876:12)
    at processTicksAndRejections (internal/process/task_queues.js:81:21)
Terminating platform threads...
close even triggered e54b0b9
platforms:  undefined
TypeError: Cannot read property 'values' of undefined
    at Timeout._onTimeout (/sockethub/packages/sockethub/bin/sockethub:14:62)
    at listOnTimeout (internal/timers.js:531:17)
    at processTimers (internal/timers.js:475:7)
Unable to reliably terminate platforms, please check your process list for orphaned `platform.js` threads.

…ecret is deleted

silverbucket · 2020-06-15T23:39:42Z

@galfert thanks for catching these, can you try again? should be fixed this time.

…or failures.

galfert · 2020-06-16T11:07:06Z

Yes, warnings are gone and server doesn't crash anymore on auth failures 👍

I can also connect to both IRC and XMPP at the same time 🎉

Sending messages also works, but the client doesn't seem to receive messages anymore. E.g. the list of users of a room when asking for attendance or just normal chat messages.

When sending a message from another IRC client, I can see this in the SH logs:

sockethub:platform:irc:6ad516a calling sendToClient for galfert@irc.freenode.net [ 'moxego@irc.freenode.net' ] +51s
sockethub:platform sending to client +51s

But in Hyperchannel, I don't see any incoming message.

I'm not 100% sure, but I think yesterday I received the room attendance messages at least. Hadn't tried chat messages yet, because of the errors.

raucao · 2020-06-30T08:25:48Z

@silverbucket Did you see @galfert's comment?

silverbucket · 2020-07-07T14:36:01Z

Yeah, I saw the comment, and talked with @galfert about it on a call a few weeks ago, I just haven't had any time to sit down and work on anything, but hopefully very soon.

silverbucket · 2020-07-08T22:15:42Z

Alright, got the incoming messages (platform to client direction) working. Things should work fine, but the checkboxes in the PR desc still need to be addressed (or in the case of the 3rd one, decided upon). I will work on 1 & 2 very soon, would love to have some help/suggestions on 3, it's a standard parent/process issue and nothing sockethub specific.

galfert · 2020-07-09T13:55:48Z

Yay, I can confirm that I can send and receive messages with Hyperchannel now, in both IRC and XMPP, without problems.

Great work! 👍

raucao · 2020-07-22T12:52:37Z

Some outstanding things to resolve:

@silverbucket Are those really important to solve in this same PR, instead of smaller, new ones? I don't see anything in the list that seems like a real blocker for us to deploy a potential new master from this for Kosmos Chat.

(Merging to master doesn't have to mean the same as tagging a release.)

silverbucket · 2020-07-22T17:27:46Z

@skddc the first and second issues (updating changed credentials and handling single-instance platforms) should be fixed as this PR breaks that functionality. The second is almost complete. I will hopefully have time to complete these two issues next week as I'm out of town this weekend.

raucao · 2020-07-22T18:27:03Z

Why not just create issues for those and document them as known issues in the README? As long as you don't tag a new release, it won't impact anyone using that functionality at the moment.

In fact, kredits naturally incentivizes smaller, more focused PRs (that are easier to review and merge as well), by limiting the largest label to 5000. Smaller PRs also make it much easier to work with more people in parallel, as anyone wanting to use this branch for feature development, would now have to branch off an unstable branch.

galfert · 2020-07-23T15:02:04Z

I would also prefer to merge this PR now, because it's working much better than the current master already.

Also, I think I would like to give task number 3 (killing the child processes when the main process dies) a try tomorrow. And I would rather create a new PR against master than against this PR.

silverbucket · 2020-07-23T20:33:07Z

Sounds good to me! Going to merge this PR, I created those 3 issues and linked them to a v3.2 project, along with other related issues. Thanks guys for all the help with this!

silverbucket added 3 commits March 27, 2020 23:54

worker now forks a process starting with platform.js which manages an…

9ab52a9

… instance of the platform it was initialized to manage

send callback message from platform to worker when the command has co…

49c837c

…mpleted

better handling of failures, job from queue is resolved, and error me…

31be623

…ssage sent to client without disconnecting the socket.

silverbucket added type:feature kredits-3 Large contribution labels Mar 30, 2020

silverbucket requested review from raucao and galfert March 30, 2020 18:00

silverbucket self-assigned this Mar 30, 2020

Publish

1052bec

- sockethub-platform-xmpp@3.2.2-alpha.0 - sockethub@3.2.0-alpha.0

raucao reviewed Mar 30, 2020

View reviewed changes

silverbucket added 12 commits April 24, 2020 02:15

aqdd hash function to crypto

57334a8

include config in the platform record

7a9ac13

sockethub core process now registers incoming connections with the pr…

7c3f051

…ocess manager and sends jobs to a special id to route to the platform thread

typing

8047559

process manager initial checking

7cbbcfc

platform get parentId as another command-line param, and listens on t…

1d5d37b

…he queue for jobs

adjustments to cleanup of a threadsrc/resource-manager.ts

a2b5fa4

adjustments to cleanup of a thread

42768f9

process listens to jobs, and sends results back in the queue

42e18c3

when the process starts up, use IPC to send the parent secrets for de…

67a7bef

…crypting jobs

init process manager with secrets

6041952

whitespace

0526b4d

silverbucket added 2 commits June 14, 2020 12:54

typo and placeholder function

7a411da

some comments and syntax

517f172

silverbucket added 5 commits June 15, 2020 19:05

dist

638db52

handle messages, and placeholder for credential updates/changes

fe0f366

delete sessionSecret after getting it

9659004

pass in actor param if provided

03a3771

todo note

ff9e494

fix linting

a20d3eb

properly handle error messages during platform crash, ensure sessionS…

9a5d613

…ecret is deleted

silverbucket added 2 commits June 16, 2020 01:52

dist

5e237ff

remove platform instance when an error is received, errors are just f…

1db4497

…or failures.

silverbucket added 4 commits July 8, 2020 23:44

build

ce79c39

send messages to client, add some typings

5adae88

build

10f3a8c

more typing cleanup

1d21fbb

silverbucket added the state:in-progress label Jul 8, 2020

silverbucket merged commit 2fd7f73 into master Jul 23, 2020

galfert deleted the feature/domain_to_process branch July 23, 2020 20:58

galfert mentioned this pull request Jul 29, 2020

Problems with joining a channel and sending/receiving messages #251

Closed

Switch from node.js domains to child process #258

Switch from node.js domains to child process #258

Uh oh!

Conversation

silverbucket commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverbucket commented Mar 30, 2020

Uh oh!

raucao commented Mar 30, 2020

Uh oh!

raucao commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raucao left a comment

Choose a reason for hiding this comment

Uh oh!

raucao Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

raucao Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

raucao commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverbucket commented Mar 30, 2020

Uh oh!

raucao commented May 5, 2020

Uh oh!

silverbucket commented Jun 15, 2020

Uh oh!

galfert commented Jun 15, 2020

Uh oh!

silverbucket commented Jun 15, 2020

Uh oh!

galfert commented Jun 16, 2020

Uh oh!

raucao commented Jun 30, 2020

Uh oh!

silverbucket commented Jul 7, 2020

Uh oh!

silverbucket commented Jul 8, 2020

Uh oh!

galfert commented Jul 9, 2020

Uh oh!

raucao commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverbucket commented Jul 22, 2020

Uh oh!

raucao commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

galfert commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverbucket commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

silverbucket commented Mar 30, 2020 •

edited

Loading

raucao commented Mar 30, 2020 •

edited

Loading

raucao commented Mar 30, 2020 •

edited

Loading

raucao commented Jul 22, 2020 •

edited

Loading

raucao commented Jul 22, 2020 •

edited

Loading

galfert commented Jul 23, 2020 •

edited

Loading

silverbucket commented Jul 23, 2020 •

edited

Loading