Skip to content

Commit

Permalink
Ensure the supervisor can start even when http_proxy is set (#5825)
Browse files Browse the repository at this point in the history
We have had several user reports of the kernel supervisor failing to
start because of HTTP proxy settings (specifically via the `http_proxy`
environment variable). This happens because the Node request library we
use reads the `http_proxy` environment variable and always attempts to
use the proxy (even, surprisingly enough, for local connections).

The fix here is -- when `http_proxy` is set -- to amend the `no_proxy`
environment variable so that the proxy is not used when connecting to
the supervisor.

Partially addresses #5481.

### QA Notes

It can be a little tricky to set environment variables for Positron. The
easiest way is probably to set them in a terminal and then launch
Positron from that terminal.
  • Loading branch information
jmcphers authored Dec 19, 2024
1 parent 5d407ae commit dbcb3b6
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 3 deletions.
12 changes: 11 additions & 1 deletion extensions/positron-supervisor/src/KallichoreAdapterApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export class KCApi implements KallichoreAdapterApi {
*
* @returns A promise that resolves when the Kallichore server is online.
*/
async ensureStarted(): Promise<void> {
public async ensureStarted(): Promise<void> {

// If the server is already started, we're done
if (this._started.isOpen()) {
Expand Down Expand Up @@ -290,6 +290,16 @@ export class KCApi implements KallichoreAdapterApi {
// Wait for the terminal to start and get the PID
await terminal.processId;

// If an HTTP proxy is set, exempt the supervisor from it; since this
// is a local server, we generally don't want to route it through a
// proxy
if (process.env.http_proxy) {
// Add the server's port to the no_proxy list, amending it if it
// already exists
process.env.no_proxy = (process.env.no_proxy ? process.env.no_proxy + ',' : '') + `localhost:${port}`;
this._log.appendLine(`HTTP proxy set to ${process.env.http_proxy}; setting no_proxy to ${process.env.no_proxy} to exempt supervisor`);
}

// Establish the API
this._api.basePath = `http://localhost:${port}`;
this._api.setDefaultAuthentication(bearer);
Expand Down
2 changes: 1 addition & 1 deletion extensions/positron-supervisor/src/test/README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Launch tests by running this from the repository root:

```sh
yarn test-extension -l positron-supervisor
npm run test-extension -l positron-supervisor
```
30 changes: 29 additions & 1 deletion extensions/positron-supervisor/src/test/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,35 @@ import { API_INSTANCE } from '../extension';
// Basic test to ensure the server can start and return status, and is the
// correct version as specified in the package.json.
suite('Server', () => {
test('Server starts and connects', async () => {
// Ensure that the server connects even when the http_proxy environment
// variable is set.
//
// NOTE: This test must run first because we only examine this value when
// the server is started; subsequent tests will use the warm server. We
// could improve this by adding some scaffolding to use a fresh server
// instance for every test.
test('Server connects when http_proxy is set', async () => {
// Set the http_proxy environment variable to a non-existent proxy
const oldHttpProxy = process.env.http_proxy;
process.env.http_proxy = 'http://example.com:234';

// Start the server and connect to it
try {
await API_INSTANCE.ensureStarted();
const status = await API_INSTANCE.serverStatus();

// Sanity check a response field
assert.strictEqual(status.sessions, 0);
} finally {
// Reset the http_proxy environment variable
process.env.http_proxy = oldHttpProxy;
}
});

test('Server version is correct', async () => {

// Start the server and connect to it
await API_INSTANCE.ensureStarted();
const status = await API_INSTANCE.serverStatus();

// Read the package.json file to get the version
Expand All @@ -20,4 +47,5 @@ suite('Server', () => {
// package.json version
assert.strictEqual(status.version, pkg.positron.binaryDependencies.kallichore);
});

});

0 comments on commit dbcb3b6

Please sign in to comment.