Repository: solid/community-server
Pull request #653 (2021-03-04 16:26:40)

Hangs when certain arguments are provided

RubenVerborgh: Closes #652
RubenVerborgh: Maybe we can also make it sync again? Cfr. https://github.com/solid/community-server/pull/640#discussion_r587547817
joachimvh: > Maybe we can also make it sync again? Cfr. [#640 (comment)](https://github.com/solid/community-server/pull/640#discussion_r587547817)

The hanging was not because of it being a promise (which actually surprised me, would have expected an uncaught exception error here), but yes, probably still best to make this sync just to be sure.
arnevandoorslaer: > > Maybe we can also make it sync again? Cfr. [#640 (comment)](https://github.com/solid/community-server/pull/640#discussion_r587547817)
> 
> The hanging was not because of it being a promise (which actually surprised me, would have expected an uncaught exception error here), but yes, probably still best to make this sync just to be sure.

Should this be made in this pr? Isn't it better to make it into a separate pr because it doesn't really have anything to do with this one?
joachimvh: > Should this be made in this pr? Isn't it better to make it into a separate pr because it doesn't really have anything to do with this one?

It's only a minor change (pretty much undo the async stuff I asked you to add 😅 and add a comment above the run function) so I would just add it here as well.

Patch:
diff --git a/src/init/CliRunner.ts b/src/init/CliRunner.ts
index 00a63aa92..4f504aafa 100644
--- a/src/init/CliRunner.ts
+++ b/src/init/CliRunner.ts
@@ -13,10 +13,11 @@ export class CliRunner {
 
   /**
    * Generic run function for starting the server from a given config
+   * Made run to be non-async to lower the chance of unhandled promise rejection errors in the future.
    * @param args - Command line arguments.
    * @param stderr - Standard error stream.
    */
-  public async run({
+  public run({
     argv = process.argv,
     stderr = process.stderr,
   }: {
@@ -24,7 +25,7 @@ export class CliRunner {
     stdin?: ReadStream;
     stdout?: WriteStream;
     stderr?: WriteStream;
-  } = {}): Promise<void> {
+  } = {}): void {
     // Parse the command-line arguments
     const { argv: params } = yargs(argv.slice(2))
       .usage('node ./bin/server.js [args]')
@@ -44,6 +45,10 @@ export class CliRunner {
             if (!args[key]) {
               throw new Error(`Missing value for argument "${key}"`);
             }
+            // Check if the argument only has 1 value
+            if (Array.isArray(args[key])) {
+              throw new Error(`Multiple values were provided for: "${key}", [${args[key]}]`);
+            }
           }
         }
         return true;
@@ -71,7 +76,7 @@ export class CliRunner {
     const variables = this.createVariables(params);
 
     // Create and execute the server initializer
-    await this.createInitializer(loaderProperties, configFile, variables)
+    this.createInitializer(loaderProperties, configFile, variables)
       .then(
         async(initializer): Promise<void> => initializer.handleSafe(),
         (error: Error): void => {


Test patch:
diff --git a/test/unit/init/CliRunner.test.ts b/test/unit/init/CliRunner.test.ts
index 8ef803fa3..a2efef334 100644
--- a/test/unit/init/CliRunner.test.ts
+++ b/test/unit/init/CliRunner.test.ts
@@ -31,10 +31,15 @@ describe('CliRunner', (): void => {
   });
 
   it('starts the server with default settings.', async(): Promise<void> => {
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [ 'node', 'script' ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(ComponentsManager.build).toHaveBeenCalledTimes(1);
     expect(ComponentsManager.build).toHaveBeenCalledWith({
       dumpErrorState: true,
@@ -64,7 +69,7 @@ describe('CliRunner', (): void => {
   });
 
   it('accepts abbreviated flags.', async(): Promise<void> => {
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [
         'node', 'script',
         '-b', 'http://pod.example/',
@@ -79,6 +84,11 @@ describe('CliRunner', (): void => {
       ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(ComponentsManager.build).toHaveBeenCalledTimes(1);
     expect(ComponentsManager.build).toHaveBeenCalledWith({
       dumpErrorState: true,
@@ -105,7 +115,7 @@ describe('CliRunner', (): void => {
   });
 
   it('accepts full flags.', async(): Promise<void> => {
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [
         'node', 'script',
         '--baseUrl', 'http://pod.example/',
@@ -120,6 +130,11 @@ describe('CliRunner', (): void => {
       ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(ComponentsManager.build).toHaveBeenCalledTimes(1);
     expect(ComponentsManager.build).toHaveBeenCalledWith({
       dumpErrorState: true,
@@ -147,10 +162,15 @@ describe('CliRunner', (): void => {
 
   it('exits with output to stderr when instantiation fails.', async(): Promise<void> => {
     manager.instantiate.mockRejectedValueOnce(new Error('Fatal'));
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [ 'node', 'script' ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(write).toHaveBeenCalledTimes(2);
     expect(write).toHaveBeenNthCalledWith(1,
       expect.stringMatching(/^Error: could not instantiate server from .*config-default\.json/u));
@@ -163,7 +183,12 @@ describe('CliRunner', (): void => {
 
   it('exits without output to stderr when initialization fails.', async(): Promise<void> => {
     initializer.handleSafe.mockRejectedValueOnce(new Error('Fatal'));
-    await new CliRunner().run();
+    new CliRunner().run();
+
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
 
     expect(write).toHaveBeenCalledTimes(0);
 
@@ -171,34 +196,65 @@ describe('CliRunner', (): void => {
   });
 
   it('exits when unknown options are passed to the main executable.', async(): Promise<void> => {
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [
         'node', 'script', '--foo',
       ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(exit).toHaveBeenCalledTimes(1);
     expect(exit).toHaveBeenCalledWith(1);
   });
 
   it('exits when no value is passed to the main executable for an argument.', async(): Promise<void> => {
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [
         'node', 'script', '-s',
       ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(exit).toHaveBeenCalledTimes(1);
     expect(exit).toHaveBeenCalledWith(1);
   });
 
   it('exits when unknown parameters are passed to the main executable.', async(): Promise<void> => {
-    await new CliRunner().run({
+    new CliRunner().run({
       argv: [
         'node', 'script', 'foo', 'bar', 'foo.txt', 'bar.txt',
       ],
     });
 
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
+    expect(exit).toHaveBeenCalledTimes(1);
+    expect(exit).toHaveBeenCalledWith(1);
+  });
+
+  it('exits when multiple values for a parameter are passed.', async(): Promise<void> => {
+    new CliRunner().run({
+      argv: [
+        'node', 'script', '-ll',
+      ],
+    });
+
+    // Wait until initializer has been called, because we can't await CliRunner.run.
+    await new Promise((resolve): void => {
+      setImmediate(resolve);
+    });
+
     expect(exit).toHaveBeenCalledTimes(1);
     expect(exit).toHaveBeenCalledWith(1);
   });