Skip to content

Commit

Permalink
Attempt to de-asyncify serverUrl computation
Browse files Browse the repository at this point in the history
  • Loading branch information
@fflorent
fflorent committed Nov 5, 2024
1 parent 00ed764 commit 9530a5f
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 72 deletions.
34 changes: 18 additions & 16 deletions test/server/lib/DocApi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
getDocApiUsageKeysToIncr,
WebhookSubscription
} from 'app/server/lib/DocApi';
import {delayAbort} from 'app/server/lib/serverUtils';
import {delayAbort, getAvailablePort} from 'app/server/lib/serverUtils';
import axios, {AxiosRequestConfig, AxiosResponse} from 'axios';
import {delay} from 'bluebird';
import {assert} from 'chai';
Expand Down Expand Up @@ -141,7 +141,7 @@ describe('DocApi', function () {
GRIST_DATA_DIR: dataDir
};
home = docs = await TestServer.startServer('home,docs', tmpDir, suitename, additionalEnvConfiguration);
homeUrl = serverUrl = await home.getServerUrl();
homeUrl = serverUrl = home.serverUrl;
hasHomeApi = true;
});
testDocApi({webhooksTestPort});
Expand All @@ -155,7 +155,7 @@ describe('DocApi', function () {
GRIST_ANON_PLAYGROUND: 'false'
};
home = docs = await TestServer.startServer('home,docs', tmpDir, suitename, additionalEnvConfiguration);
homeUrl = serverUrl = await home.getServerUrl();
homeUrl = serverUrl = home.serverUrl;
hasHomeApi = true;
});

Expand All @@ -176,7 +176,7 @@ describe('DocApi', function () {
};

home = await TestServer.startServer('home', tmpDir, suitename, additionalEnvConfiguration);
homeUrl = serverUrl = await home.getServerUrl();
homeUrl = serverUrl = home.serverUrl;
docs = await TestServer.startServer('docs', tmpDir, suitename, additionalEnvConfiguration, homeUrl);
hasHomeApi = true;
});
Expand All @@ -185,30 +185,32 @@ describe('DocApi', function () {

describe('behind a reverse-proxy', function () {
async function setupServersWithProxy(suitename: string, overrideEnvConf?: NodeJS.ProcessEnv) {
const proxy = new TestServerReverseProxy();
const proxy = await TestServerReverseProxy.build();
const additionalEnvConfiguration = {
ALLOWED_WEBHOOK_DOMAINS: `example.com,localhost:${webhooksTestPort}`,
GRIST_DATA_DIR: dataDir,
APP_HOME_URL: await proxy.getServerUrl(),
APP_HOME_URL: proxy.serverUrl,
GRIST_ORG_IN_PATH: 'true',
GRIST_SINGLE_PORT: '0',
...overrideEnvConf
};

const home = new TestServer('home', tmpDir, suitename);
await home.start(await home.getServerUrl(), additionalEnvConfiguration);
const docs = new TestServer('docs', tmpDir, suitename);
await docs.start(await home.getServerUrl(), {
const homePort = await getAvailablePort(parseInt(process.env.GET_AVAILABLE_PORT_START || '8080', 10));
const home = new TestServer('home', homePort, tmpDir, suitename);
await home.start(home.serverUrl, additionalEnvConfiguration);
const docPort = await getAvailablePort(parseInt(process.env.GET_AVAILABLE_PORT_START || '8080', 10));
const docs = new TestServer('docs', docPort, tmpDir, suitename);
await docs.start(home.serverUrl, {
...additionalEnvConfiguration,
APP_DOC_URL: `${await proxy.getServerUrl()}/dw/dw1`,
APP_DOC_INTERNAL_URL: await docs.getServerUrl(),
APP_DOC_URL: `${proxy.serverUrl}/dw/dw1`,
APP_DOC_INTERNAL_URL: docs.serverUrl,
});

proxy.requireFromOutsideHeader();

await proxy.start(home, docs);

homeUrl = serverUrl = await proxy.getServerUrl();
homeUrl = serverUrl = proxy.serverUrl;
hasHomeApi = true;
extraHeadersForConfig = {
Origin: serverUrl,
Expand Down Expand Up @@ -281,9 +283,9 @@ describe('DocApi', function () {
GRIST_DATA_DIR: dataDir
};
home = await TestServer.startServer('home', tmpDir, suitename, additionalEnvConfiguration);
homeUrl = await home.getServerUrl();
homeUrl = home.serverUrl;
docs = await TestServer.startServer('docs', tmpDir, suitename, additionalEnvConfiguration, homeUrl);
serverUrl = await docs.getServerUrl();
serverUrl = docs.serverUrl;
hasHomeApi = false;
});
testDocApi({webhooksTestPort});
Expand Down Expand Up @@ -3469,7 +3471,7 @@ function testDocApi(settings: {
if (docs.proxiedServer) {
this.skip();
}
const docWorkerUrl = await docs.getServerUrl();
const docWorkerUrl = docs.serverUrl;
let resp = await axios.get(`${docWorkerUrl}/api/docs/${docIds.Timesheets}/tables/Table1/data`, chimpy);
assert.equal(resp.status, 200);
assert.containsAllKeys(resp.data, ['A', 'B', 'C']);
Expand Down
8 changes: 4 additions & 4 deletions test/server/lib/ManyFetches.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ describe('ManyFetches', function() {
// Without this limit, there is no pressure on node to garbage-collect, so it may use more
// memory than we expect, making the test less reliable.
NODE_OPTIONS: '--max-old-space-size=210',
}, await home.getServerUrl());
userApi = await home.makeUserApi(org, userName);
}, home.serverUrl);
userApi = home.makeUserApi(org, userName);
});

afterEach(async function() {
Expand Down Expand Up @@ -222,13 +222,13 @@ describe('ManyFetches', function() {
// function.
async function prepareGristWSConnection(docId: string): Promise<() => GristWSConnection> {
// Use cookies for access to stay as close as possible to regular operation.
const resp = await fetch(`${await home.getServerUrl()}/test/session`);
const resp = await fetch(`${home.serverUrl}/test/session`);
const sid = cookie.parse(resp.headers.get('set-cookie'))[cookieName];
if (!sid) { throw new Error('no session available'); }
await home.testingHooks.setLoginSessionProfile(sid, {name: userName, email}, org);

// Load the document html.
const pageUrl = `${await home.getServerUrl()}/o/docs/doc/${docId}`;
const pageUrl = `${home.serverUrl}/o/docs/doc/${docId}`;
const headers = {Cookie: `${cookieName}=${sid}`};
const doc = await fetch(pageUrl, {headers});
const pageBody = await doc.text();
Expand Down
4 changes: 2 additions & 2 deletions test/server/lib/UnhandledErrors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('UnhandledErrors', function() {
const server = await TestServer.startServer('home', testDir, errType, undefined, undefined, {output});

try {
assert.equal((await fetch(`${await server.getServerUrl()}/status`)).status, 200);
assert.equal((await fetch(`${server.serverUrl}/status`)).status, 200);
serverLogLines.length = 0;

// Trigger an unhandled error, and check that the server logged it and attempted cleanup.
Expand All @@ -51,7 +51,7 @@ describe('UnhandledErrors', function() {
}, 1000, 100);

// We expect the server to be dead now.
await assert.isRejected(fetch(`${await server.getServerUrl()}/status`), /(request.*failed)|(ECONNREFUSED)/);
await assert.isRejected(fetch(`${server.serverUrl}/status`), /(request.*failed)|(ECONNREFUSED)/);

} finally {
await server.stop();
Expand Down
11 changes: 5 additions & 6 deletions test/server/lib/Webhooks-Proxy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ describe('Webhooks-Proxy', function () {
await cb();

// create TestDoc as an empty doc into Private workspace
userApi = api = await home.makeUserApi(ORG_NAME);
userApi = api = home.makeUserApi(ORG_NAME);
const wid = await getWorkspaceId(api, 'Private');
docIds.TestDoc = await api.newDoc({name: 'TestDoc'}, wid);
});
Expand Down Expand Up @@ -125,7 +125,7 @@ describe('Webhooks-Proxy', function () {
describe("should work with a merged server", async () => {
setupMockServers('merged', tmpDir, async () => {
home = docs = await TestServer.startServer('home,docs', tmpDir, suitename, additionaEnvConfiguration);
serverUrl = await home.getServerUrl();
serverUrl = home.serverUrl;
});
subTestCall();
});
Expand All @@ -135,7 +135,7 @@ describe('Webhooks-Proxy', function () {
describe("should work with a home server and a docworker", async () => {
setupMockServers('separated', tmpDir, async () => {
home = await TestServer.startServer('home', tmpDir, suitename, additionaEnvConfiguration);
const homeUrl = await home.getServerUrl();
const homeUrl = home.serverUrl;
docs = await TestServer.startServer('docs', tmpDir, suitename, additionaEnvConfiguration, homeUrl);
serverUrl = homeUrl;
});
Expand All @@ -146,9 +146,8 @@ describe('Webhooks-Proxy', function () {
describe("should work directly with a docworker", async () => {
setupMockServers('docs', tmpDir, async () => {
home = await TestServer.startServer('home', tmpDir, suitename, additionaEnvConfiguration);
const homeUrl = await home.getServerUrl();
docs = await TestServer.startServer('docs', tmpDir, suitename, additionaEnvConfiguration, homeUrl);
serverUrl = await docs.getServerUrl();
docs = await TestServer.startServer('docs', tmpDir, suitename, additionaEnvConfiguration, home.serverUrl);
serverUrl = docs.serverUrl;
});
subTestCall();
});
Expand Down
75 changes: 31 additions & 44 deletions test/server/lib/helpers/TestServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {delay} from "bluebird";
import fetch from "node-fetch";
import {Writable} from "stream";
import express from "express";
import { AddressInfo } from "net";
import { isAffirmative } from "app/common/gutil";
import httpProxy from 'http-proxy';

Expand All @@ -28,8 +27,8 @@ export class TestServer {
_homeUrl?: string,
options: {output?: Writable} = {}, // Pipe server output to the given stream
): Promise<TestServer> {

const server = new this(serverTypes, tempDirectory, suitename);
const port = await getAvailablePort(parseInt(process.env.GET_AVAILABLE_PORT_START || '8080', 10));
const server = new this(serverTypes, port, tempDirectory, suitename);
await server.start(_homeUrl, customEnv, options);
return server;
}
Expand All @@ -38,15 +37,26 @@ export class TestServer {
public testingHooks: TestingHooksClient;
public stopped = false;
public get proxiedServer() { return this._proxiedServer; }
public get serverUrl() {
if (this._proxiedServer) {
throw new Error('Direct access to this test server is disallowed');
}

return `http://localhost:${this.port}`;
}

private _serverUrl: Promise<string>;
private _server: ChildProcess;
private _exitPromise: Promise<number | string>;
private _proxiedServer: boolean = false;

private readonly _defaultEnv;

constructor(private _serverTypes: string, public readonly rootDir: string, private _suiteName: string) {
constructor(
private _serverTypes: string,
public readonly port: number,
public readonly rootDir: string,
private _suiteName: string
) {
this._defaultEnv = {
GRIST_INST_DIR: this.rootDir,
GRIST_DATA_DIR: path.join(this.rootDir, "data"),
Expand All @@ -59,18 +69,6 @@ export class TestServer {
GRIST_MAX_QUEUE_SIZE: '10',
...process.env
};
this._serverUrl = new Promise((resolve) => {
return getAvailablePort(Number(process.env.GET_AVAILABLE_PORT_START || '8000')).then((port) => {
resolve(`http://localhost:${port}`);
});
});
}

public getServerUrl() {
if (this._proxiedServer) {
throw new Error('Direct access to this test server is disallowed');
}
return this._serverUrl;
}

public async start(homeUrl?: string, customEnv?: NodeJS.ProcessEnv, options: {output?: Writable} = {}) {
Expand All @@ -86,14 +84,11 @@ export class TestServer {
throw new Error(`Path of testingSocket too long: ${this.testingSocket.length} (${this.testingSocket})`);
}

const serverUrl = await this.getServerUrl();
const port = new URL(serverUrl).port;

const env: NodeJS.ProcessEnv = {
APP_HOME_URL: homeUrl,
APP_HOME_INTERNAL_URL: homeUrl,
GRIST_TESTING_SOCKET: this.testingSocket,
GRIST_PORT: port,
GRIST_PORT: String(this.port),
...this._defaultEnv,
...customEnv
};
Expand Down Expand Up @@ -121,7 +116,7 @@ export class TestServer {
.catch(() => undefined);

await this._waitServerReady();
log.info(`server ${this._serverTypes} up and listening on ${this._serverUrl}`);
log.info(`server ${this._serverTypes} up and listening on ${this.serverUrl}`);
}

public async stop() {
Expand Down Expand Up @@ -150,7 +145,7 @@ export class TestServer {
this.testingHooks = await connectTestingHooks(this.testingSocket);

// wait for check
return (await fetch(`${this._serverUrl}/status/hooks`, {timeout: 1000})).ok;
return (await fetch(`${this.serverUrl}/status/hooks`, {timeout: 1000})).ok;
} catch (err) {
log.warn("Failed to initialize server", err);
return false;
Expand All @@ -163,8 +158,8 @@ export class TestServer {
// Returns the promise for the ChildProcess's signal or exit code.
public getExitPromise(): Promise<string|number> { return this._exitPromise; }

public async makeUserApi(org: string, user: string = 'chimpy'): Promise<UserAPIImpl> {
return new UserAPIImpl(`${await this.getServerUrl()}/o/${org}`, {
public makeUserApi(org: string, user: string = 'chimpy'): UserAPIImpl {
return new UserAPIImpl(`${this.serverUrl}/o/${org}`, {
headers: {Authorization: `Bearer api_key_for_${user}`},
fetch: fetch as unknown as typeof globalThis.fetch,
newFormData: () => new FormData() as any,
Expand Down Expand Up @@ -224,21 +219,22 @@ export class TestServerReverseProxy {

public static FROM_OUTSIDE_HEADER = {"X-FROM-OUTSIDE": true};

public static async build() {
const port = await getAvailablePort(parseInt(process.env.GET_AVAILABLE_PORT_START || '8080', 10));
return new this(port);
}

public get serverUrl() { return `http://${TestServerReverseProxy.HOSTNAME}:${this.port}`; }

private _app = express();
private _proxyServer: http.Server;
private _proxy: httpProxy = httpProxy.createProxy();
private _address: Promise<AddressInfo>;
private _requireFromOutsideHeader = false;

public get stopped() { return !this._proxyServer.listening; }

public constructor() {
this._proxyServer = this._app.listen(0);
this._address = new Promise((resolve) => {
this._proxyServer.once('listening', () => {
resolve(this._proxyServer.address() as AddressInfo);
});
});
public constructor(public readonly port: number) {
this._proxyServer = this._app.listen(port);
}

/**
Expand All @@ -262,16 +258,7 @@ export class TestServerReverseProxy {
homeServer.disallowDirectAccess();
docServer.disallowDirectAccess();

log.info('proxy server running on ', await this.getServerUrl());
}

public async getAddress() {
return this._address;
}

public async getServerUrl() {
const address = await this.getAddress();
return `http://${TestServerReverseProxy.HOSTNAME}:${address.port}`;
log.info('proxy server running on ', this.serverUrl);
}

public stop() {
Expand All @@ -284,7 +271,7 @@ export class TestServerReverseProxy {
}

private async _getRequestHandlerFor(server: TestServer) {
const serverUrl = new URL(await server.getServerUrl());
const serverUrl = new URL(server.serverUrl);

return (oreq: express.Request, ores: express.Response) => {
log.debug(`[proxy] Requesting (method=${oreq.method}): ${new URL(oreq.url, serverUrl).href}`);
Expand Down

0 comments on commit 9530a5f

Please sign in to comment.