Skip to content

Commit

Permalink
Ready needs to be able to handle asynchronous code
Browse files Browse the repository at this point in the history
Summary:
A previous diff introduced the isReady state as to make it possible to serve HTML content sooner than we are ready.

This worked great on debugging. As I was testing a release, it was discovered that there was a race condition and the server was not ready when it was already handling upgrade events.

To solve this, I've added another state flag in the form of a promise.

This one, can be waited on. This is used then during upgrade events as we can safely wait until the server is ready to accept incoming connections before proceeding with the upgrade.

Problem is shown below:

{F1080003241}
{F1080003356}

Reviewed By: passy

Differential Revision: D48829453

fbshipit-source-id: e148a392bbe66dd91710e32871e270c8950e25c2
  • Loading branch information
lblasa authored and facebook-github-bot committed Aug 30, 2023
1 parent 072d618 commit 457767c
Showing 1 changed file with 42 additions and 3 deletions.
45 changes: 42 additions & 3 deletions desktop/flipper-server-core/src/server/startServer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,17 @@ const verifyAuthToken = (req: http.IncomingMessage): boolean => {
return true;
};

/**
* The following two variables are used to control when
* the server is ready to accept incoming connections.
* - isReady, is used to synchronously check if the server is
* ready or not.
* - isReadyWaitable achieves the same but is used by
* asynchronous functions which may want to wait until the
* server is ready.
*/
let isReady = false;
let isReadyWaitable: Promise<void> | undefined;

/**
* Orchestrates the creation of the HTTP server, proxy, and WS server.
Expand Down Expand Up @@ -134,18 +144,36 @@ async function startHTTPServer(config: Config): Promise<{

server.listen(config.port);

/**
* Create the promise which can be waited on. In this case,
* a reference to resolve is kept outside of the body of the promise
* so that other asynchronous functions can resolve the promise
* on its behalf.
*/
let isReadyResolver: (value: void | PromiseLike<void>) => void;
isReadyWaitable = new Promise((resolve, _reject) => {
isReadyResolver = resolve;
});

return new Promise((resolve) => {
console.log(`Starting server on http://localhost:${config.port}`);
const readyForIncomingConnections = (
serverImpl: FlipperServerImpl,
companionEnv: FlipperServerCompanionEnv,
): Promise<void> => {
attachSocketServer(socket, serverImpl, companionEnv);
/**
* At this point, the server is ready to accept incoming
* connections. Change the isReady state and resolve the
* promise so that other asychronous function become unblocked.
*/
isReady = true;
isReadyResolver();
return new Promise((resolve) => {
tracker.track('server-started', {
port: config.port,
});

resolve();
});
};
Expand Down Expand Up @@ -214,22 +242,33 @@ function attachWS(server: http.Server, config: Config) {
};

const wss = new WebSocketServer(options);
server.on('upgrade', function upgrade(request, socket, head) {
const {pathname} = parse(request.url!);
server.on('upgrade', async function upgrade(request, socket, head) {
if (!request.url) {
console.log('[flipper-server] No request URL available');
socket.destroy();
return;
}

const {pathname} = parse(request.url);

// Handled by Metro
if (pathname === '/hot') {
return;
}

if (pathname === '/') {
// Wait until the server is ready to accept incoming connections.
await isReadyWaitable;
wss.handleUpgrade(request, socket, head, function done(ws) {
wss.emit('connection', ws, request);
});
return;
}

console.error('addWebsocket.upgrade -> unknown pathname', pathname);
console.error(
'[flipper-server] Unable to upgrade, unknown pathname',
pathname,
);
socket.destroy();
});

Expand Down

0 comments on commit 457767c

Please sign in to comment.