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

Decouple terminal sessions from connections #68

Open
ericfranz opened this issue Aug 2, 2019 · 2 comments

Comments

@ericfranz
Copy link
Contributor

commented Aug 2, 2019

There are several possible issues that can result in the ssh session dying after 1 a minute or two disconnected from the network.

The problem with the app as it is written is that the ssh session

ood-shell/app.js

Lines 67 to 73 in 2a1af63

term = pty.spawn(cmd, args, {
name: 'xterm-256color',
cols: 80,
rows: 30,
cwd: cwd,
env: env
});
is coupled with the ws connection object
wss.on('connection', function connection (ws) {
or "client". So when a connection is closed due to temporary loss of network access (close laptop, switch networks, etc.) the terminal session is also closed

ood-shell/app.js

Lines 97 to 100 in 2a1af63

ws.on('close', function () {
term.end();
console.log('Closed terminal: ' + term.pid);
});

Instead, terminal sessions should be stored in a hash like object with a key being a unique identifier that is shared with the client so that the client can use that identifier when it tries to initiate a new ws connection. Then the new ws connection would have three possiblities:

  1. new connection - no unique identifier sent
  2. trying to reconnect - unique identifier sent that matches an existing ssh session, so associate that connection with the ssh session
  3. trying to reconnect to terminated ssh session - unique identifier sent that does not match an existing ssh session, return an error that can be displayed to the user!

With this work we still have the problem of the web server itself being killed by Passenger after the passenger_pool_idle_time is reached (5 minutes). Enabling sites to increase this would address the shell problem but introduce other problems as it is an option that affects all apps.

In the PUN configs, it seems that we are using default values for these:

  • passenger_pool_idle_time - default 300 (i.e. 5 minutes). The problem is this is http only context, which means it affects all apps. So if you want to increase the timeout to be an hour or 2 hours the dashboard app, the file editor, etc. up to the
  • passenger_max_pool_size - default is 6. I haven't found documentation on what happens when you try to launch an app past the passenger_max_pool_size - does it kill others or does it fail to launch? We should investigate this - maybe a separate issue?
  • passenger_sticky_sessions - default is disabled
  • passenger_max_instances_per_app (is global and applies to all instances) - default is 0. it would seem we would want 1 for this; and if this is 1 then we don't need sticky sessions enabled above
@ericfranz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

This is also a problem client side:

this.socket.onclose = this.closeTerminal.bind(this);

Instead, when the websocket is closed, a warning message should appear above the shell app - not printed to the terminal - that claims "the websocket connection failed, trying to reconnect"

@ericfranz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

And for the shell app only we could consider setting passenger_min_instances to 1:

Please note that this option does not pre-start application processes during Nginx startup. It just makes sure that when the application is first accessed:

  1. at least the given number of processes will be spawned.
  2. the given number of processes will be kept around even when processes are being idle cleaned.

Would this ensure that the for any user running a shell app that PUNs and the shell app stick around, even after users go away from the app? Forever?

We might want to consider when we would want to ensure shell apps actually get shut down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.