Skip to content

Improve shutdown for FwLiteWeb in Paratext #1992

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Improve shutdown for FwLiteWeb in Paratext #1992

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
525d58c
Improve shutdown handling for FwLiteWeb
hahn-kev Sep 9, 2025
032eea7
never throw on shutdown, just log the error and return false
hahn-kev Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions backend/FwLite/FwLiteWeb/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
using Microsoft.Extensions.Options;

//paratext won't let us change the working directory, and if it's not set correctly then loading js files doesn't work
Directory.SetCurrentDirectory(Path.GetDirectoryName(typeof(Program).Assembly.Location)!);

Check warning on line 6 in backend/FwLite/FwLiteWeb/Program.cs

View workflow job for this annotation

GitHub Actions / Publish FW Lite app for Linux 

'System.Reflection.Assembly.Location.get' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.

Check warning on line 6 in backend/FwLite/FwLiteWeb/Program.cs

View workflow job for this annotation

GitHub Actions / Publish FW Lite app for Linux 

'System.Reflection.Assembly.Location.get' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
var app = FwLiteWebServer.SetupAppServer(new() {Args = args});
await using (app)
{
Expand All @@ -15,6 +15,14 @@
var url = app.Urls.First();
LocalAppLauncher.LaunchBrowser(url);
}
//windows is dumb and so you can't send SIGINT to a process, so we need to listen for a shutdown command
_ = Task.Run(async () =>
{
// Wait for the "shutdown" command from stdin
while (await Console.In.ReadLineAsync() is not "shutdown") { }

await app.StopAsync();
});
Comment on lines +18 to +25
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle EOF and be resilient to whitespace/case; avoid a potential busy loop on closed stdin.

If stdin closes (parent dies or doesn’t pipe input), ReadLineAsync() returns null and the current loop spins forever without calling StopAsync(). Also, be lenient to trailing whitespace/casing.

Apply this diff:

-    //windows is dumb and so you can't send SIGINT to a process, so we need to listen for a shutdown command
-    _ = Task.Run(async () =>
-         {
-             // Wait for the "shutdown" command from stdin
-             while (await Console.In.ReadLineAsync() is not "shutdown") { }
-
-             await app.StopAsync();
-         });
+    // Windows can't receive SIGINT as a child; listen for a "shutdown" command on stdin.
+    _ = Task.Run(async () =>
+    {
+        string? line;
+        while ((line = await Console.In.ReadLineAsync()) is not null
+               && !string.Equals(line.Trim(), "shutdown", StringComparison.OrdinalIgnoreCase))
+        { /* keep waiting */ }
+
+        await app.StopAsync();
+    });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//windows is dumb and so you can't send SIGINT to a process, so we need to listen for a shutdown command
_ = Task.Run(async () =>
{
// Wait for the "shutdown" command from stdin
while (await Console.In.ReadLineAsync() is not "shutdown") { }
await app.StopAsync();
});
// Windows can't receive SIGINT as a child; listen for a "shutdown" command on stdin.
_ = Task.Run(async () =>
{
string? line;
while ((line = await Console.In.ReadLineAsync()) is not null
&& !string.Equals(line.Trim(), "shutdown", StringComparison.OrdinalIgnoreCase))
{ /* keep waiting */ }
await app.StopAsync();
});


await app.WaitForShutdownAsync();
}
26 changes: 23 additions & 3 deletions platform.bible-extension/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ export async function activate(context: ExecutionActivationContext): Promise<voi
// Services
await entryService,
// Other cleanup
() => fwLiteProcess?.kill(),
() => shutdownFwLite(fwLiteProcess),
);

logger.info('FieldWorks Lite is finished activating!');
Expand All @@ -275,8 +275,7 @@ function launchFwLiteFwLiteWeb(context: ExecutionActivationContext) {
context.executionToken,
binaryPath,
['--urls', baseUrl, '--FwLiteWeb:OpenBrowser=false', '--FwLiteWeb:CorsAllowAny=true'],
// eslint-disable-next-line no-null/no-null
{ stdio: [null, 'pipe', 'pipe'] },
{ stdio: ['pipe', 'pipe', 'pipe'] },
);
fwLiteProcess.once('exit', (code, signal) => {
logger.info(`[FwLiteWeb]: exited with code '${code}', signal '${signal}'`);
Expand All @@ -293,3 +292,24 @@ function launchFwLiteFwLiteWeb(context: ExecutionActivationContext) {
}
return { baseUrl, fwLiteProcess };
}

function shutdownFwLite(fwLiteProcess: ReturnType<typeof launchFwLiteFwLiteWeb>['fwLiteProcess']): Promise<boolean> {
return new Promise((resolve, reject) => {
fwLiteProcess.once('exit', (code, signal) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make the fwLiteProcess.once('exit', ... in lines 280-282 unnecessary?

if (code === 0) {
resolve(true);
} else {
reject(new Error(`FwLiteWeb exited with code ${code}, signal ${signal}`));
}
});
fwLiteProcess.once('error', (error) => {
reject(error);
});

fwLiteProcess.stdin.write('shutdown\n');
fwLiteProcess.stdin.end();
setTimeout(() => {
fwLiteProcess.kill('SIGKILL');
}, 10000);
});
}
Copy link

@coderabbitai coderabbitai bot Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Make shutdown idempotent, clear the kill timer, and avoid rejecting on forced-kill.

Today:

  • A forced kill after 10s will likely cause exit with non-zero/Signal and the Promise rejects, which can bubble as an activation teardown error.
  • The kill timer isn’t cleared on early exit.
  • If the process already exited, the Promise never resolves.
  • If stdin is unavailable, write() will error.

Refactor to: resolve(true) on clean or forced termination, resolve(false) on error, clear the timer, and short-circuit if already exited. Also guard stdin.

Apply this diff:

-function shutdownFwLite(fwLiteProcess: ReturnType<typeof launchFwLiteFwLiteWeb>['fwLiteProcess']): Promise<boolean> {
-  return new Promise((resolve, reject) => {
-    fwLiteProcess.once('exit', (code, signal) => {
-      if (code === 0) {
-        resolve(true);
-      } else {
-        reject(new Error(`FwLiteWeb exited with code ${code}, signal ${signal}`));
-      }
-    });
-    fwLiteProcess.once('error', (error) => {
-      reject(error);
-    });
-
-    fwLiteProcess.stdin.write('shutdown\n');
-    fwLiteProcess.stdin.end();
-    setTimeout(() => {
-      fwLiteProcess.kill('SIGKILL');
-    }, 10000);
-  });
-}
+function shutdownFwLite(
+  fwLiteProcess: ReturnType<typeof launchFwLiteFwLiteWeb>['fwLiteProcess'],
+): Promise<boolean> {
+  return new Promise((resolve) => {
+    // Already exited?
+    if (fwLiteProcess.exitCode !== null) {
+      return resolve(fwLiteProcess.exitCode === 0);
+    }
+
+    let forced = false;
+    const killTimer = setTimeout(() => {
+      forced = true;
+      try { fwLiteProcess.kill('SIGKILL'); } catch { /* ignore */ }
+    }, 10_000);
+
+    const onExit = (code: number | null) => {
+      clearTimeout(killTimer);
+      // Treat forced kill as a successful shutdown to avoid failing deactivation.
+      resolve(forced || code === 0);
+    };
+    const onError = () => {
+      clearTimeout(killTimer);
+      resolve(false);
+    };
+
+    fwLiteProcess.once('exit', onExit);
+    fwLiteProcess.once('error', onError);
+
+    if (fwLiteProcess.stdin && !fwLiteProcess.stdin.destroyed) {
+      try {
+        fwLiteProcess.stdin.write('shutdown\n');
+        fwLiteProcess.stdin.end();
+      } catch {
+        // Fall back to forced termination if stdin write fails.
+        forced = true;
+        try { fwLiteProcess.kill('SIGKILL'); } catch { /* ignore */ }
+      }
+    } else {
+      forced = true;
+      try { fwLiteProcess.kill('SIGKILL'); } catch { /* ignore */ }
+    }
+  });
+}

Note: On Windows, Node emulates signals; child_process.kill('SIGKILL') forces termination and cannot be handled. This is fine for the fallback. (github.com, nodejs.org)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function shutdownFwLite(fwLiteProcess: ReturnType<typeof launchFwLiteFwLiteWeb>['fwLiteProcess']): Promise<boolean> {
return new Promise((resolve, reject) => {
fwLiteProcess.once('exit', (code, signal) => {
if (code === 0) {
resolve(true);
} else {
reject(new Error(`FwLiteWeb exited with code ${code}, signal ${signal}`));
}
});
fwLiteProcess.once('error', (error) => {
reject(error);
});
fwLiteProcess.stdin.write('shutdown\n');
fwLiteProcess.stdin.end();
setTimeout(() => {
fwLiteProcess.kill('SIGKILL');
}, 10000);
});
}
function shutdownFwLite(
fwLiteProcess: ReturnType<typeof launchFwLiteFwLiteWeb>['fwLiteProcess'],
): Promise<boolean> {
return new Promise((resolve) => {
// Already exited?
if (fwLiteProcess.exitCode !== null) {
return resolve(fwLiteProcess.exitCode === 0);
}
let forced = false;
const killTimer = setTimeout(() => {
forced = true;
try { fwLiteProcess.kill('SIGKILL'); } catch { /* ignore */ }
}, 10_000);
const onExit = (code: number | null) => {
clearTimeout(killTimer);
// Treat forced kill as a successful shutdown to avoid failing deactivation.
resolve(forced || code === 0);
};
const onError = () => {
clearTimeout(killTimer);
resolve(false);
};
fwLiteProcess.once('exit', onExit);
fwLiteProcess.once('error', onError);
if (fwLiteProcess.stdin && !fwLiteProcess.stdin.destroyed) {
try {
fwLiteProcess.stdin.write('shutdown\n');
fwLiteProcess.stdin.end();
} catch {
// Fall back to forced termination if stdin write fails.
forced = true;
try { fwLiteProcess.kill('SIGKILL'); } catch { /* ignore */ }
}
} else {
forced = true;
try { fwLiteProcess.kill('SIGKILL'); } catch { /* ignore */ }
}
});
}
🤖 Prompt for AI Agents 
In platform.bible-extension/src/main.ts around lines 296-315, make
shutdownFwLite idempotent and robust: short-circuit and immediately resolve if
the child already has an exitCode or is killed; install a single timeout kill
timer and clear it when the process exits or an error occurs; on 'exit'
resolve(true) for clean (code === 0) or forced termination (non-null signal or
non-zero code after our kill) instead of rejecting, and resolve(false) only for
genuine errors; on 'error' resolve(false) (do not reject); guard writes to stdin
by checking fwLiteProcess.stdin && fwLiteProcess.stdin.writable and wrapping
write/end in try/catch; ensure listeners/timer are cleaned up exactly once by
using a local finished flag to short-circuit further callbacks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already did this

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hahn-kev, I can see in the current code that the shutdownFwLite function still has the original implementation from the diff. Looking at the current version, it still:

  • Uses reject instead of only resolve
  • Doesn't check if the process has already exited
  • Doesn't clear the kill timer on exit/error
  • Doesn't guard the stdin write operations

The function currently looks like this:

function shutdownFwLite(fwLiteProcess: ReturnType<typeof launchFwLiteFwLiteWeb>['fwLiteProcess']): Promise<boolean> {
  return new Promise((resolve, reject) => {
    fwLiteProcess.once('exit', (code, signal) => {
      if (code === 0) {
        resolve(true);
      } else {
        reject(new Error(`FwLiteWeb exited with code ${code}, signal ${signal}`));
      }
    });
    fwLiteProcess.once('error', (error) => {
      reject(error);
    });

    fwLiteProcess.stdin.write('shutdown\n');
    fwLiteProcess.stdin.end();
    setTimeout(() => {
      fwLiteProcess.kill('SIGKILL');
    }, 10000);
  });
}

Would you like me to verify if there are any recent commits that haven't been reflected in this view, or are you planning to implement the suggested improvements in a follow-up commit?

Loading