Miscellaneous issues with razzle

[therealgilles]

I have noticed a few miscellaneous issues with razzle. I'm not sure how to best report them, so I opened this bug report.

1. When using verbose, webpack-dev-server displays this message where the from is always 'undefined'.

ℹ ｢wds｣: webpack output is served from undefined

2. When I was compiled my project at first, razzle said the client was compiled with errors but never displayed what the errors were. It would process to compile the server and also reported errors there without displayed them either. An error would usually show up at runtime later on. It would be great if compile errors could be reported.

3. The console message output of razzle is displayed with colors. Yet the colors from the server console messages are removed when messages are displayed. I use the 'debug' npm package and colors are very useful when debugging.

Semi-related, when the server (or proxy if any) are issuing messages, the client compile looks like this:

● Client █████████████████████████ building (69%) 1237/1254 modules 17 active

● Client █████████████████████████ building (69%) 1258/1271 modules 13 active

● Client █████████████████████████ building (68%) 1281/1314 modules 33 active

● Client █████████████████████████ building (68%) 1300/1327 modules 27 active

Not sure there is an easy to deal with that without some complex terminal split view.

4. I had an instance when one of the servers was still running after pressing ^C to interrupt and I had to kill the process manually.

[fivethreeo]

2. I think happens when there is a webpack error and not a code error.

[fivethreeo]

1. fixed in canary now

[fivethreeo]

But wds does not output if the server does.. hrm

[fivethreeo]

4. Return here? https://github.com/jaredpalmer/razzle/blob/canary/packages/razzle-start-server-webpack-plugin/src/StartServerPlugin.js#L117

[fivethreeo]

3. Is a issue since we don't have any control of the underlying libraries

[therealgilles]

3. Do you know at what level color code characters would get removed? Is the process all javascript or is some part going through a shell command at some point?

4. Not sure. Signals to capture for exit are usually SIGINT an SIGTERM. It happened a couple of times but I cannot really pinpoint why it happened in those occasions and not others. A hammer-type solution would be to run a process launcher like pm2 on top.

[fivethreeo]

https://github.com/jaredpalmer/razzle/blob/canary/packages/razzle-start-server-webpack-plugin/src/StartServerPlugin.js#L165

[fivethreeo]

https://github.com/jaredpalmer/razzle/blob/canary/packages/razzle-start-server-webpack-plugin/src/StartServerPlugin.js#L172

[fivethreeo]

https://github.com/jaredpalmer/razzle/blob/canary/packages/razzle-start-server-webpack-plugin/src/StartServerPlugin.js#L63

[therealgilles]

Thanks for all the pointers. The child process stdout/stderr are piped so the node process does not see a tty and, in my case, the debug node module disables color code output. I was able to force-enable colors by adding this to .env.development:

# Enable colors for the debug node module
DEBUG_COLORS=true

so that's one issue fixed :)

[therealgilles]

For some reason, the messages at the beginning come out with an extra line break like this:

  httpxServer:info Creating development server... +3ms

  httpxServer:log ... http server created +0ms
  websocket:log Creating websocket server... +0ms

  websocket:log ... ws server created over http +1ms

but then later on, the extra line breaks go away, or only happen once in a while. I see all the messages are coming through _worker_error, so on stderr. Not sure why they don't come out on stdout instead. But anyway, wondering if it could be useful to filter out line breaks with something like this:

    worker.stdout.on('data', data => this._worker_info(data.toString().replace(/(\r\n|\n|\r)$/, '')));
    worker.stderr.on('data', data => this._worker_error(data.toString().replace(/(\r\n|\n|\r)$/, '')));

console.[log|error] will add the line break back anyway.

[therealgilles]

Another option would be to use process.[stdout|stderr].write() instead.

[therealgilles]

Oh I think I now understand why I don't have extra line breaks everywhere. Depending on the pipe buffering, some messages are transferred together and therefore don't have extra line breaks between them. Once the pipe is drained, there will be an extra line break. So I would conclude this is an (aesthetic) bug right now.

Proposal:

  _worker_error(msg, ...args) {
    process.stderr.write(msg);
    // or
    // console.error(msg.replace(/(\r\n|\n|\r)$/, ''));
  }

  _worker_info(msg, ...args) {
    process.stdout.write(msg);
    // or
    // console.log(msg.replace(/(\r\n|\n|\r)$/, ''));
  }

[fivethreeo]

If you want to try hacking on this, I have a workflow:

npm install -g yalc
git clone https://github.com/jaredpalmer/razzle.git
cd razzle
git checkout -B canary
NODE_ENV=development yarn install ---ignore-engines
yarn try-example basic
# or any example in the examples dir
cd example
yarn start
yarn build

Make changes to razzle and run:

yarn updateyalc
yarn build
yarn start

In the example dir to use your modified razzle

You can even add dependencies in the example dir with yarn

[therealgilles]

4. I observe that node keeps running when the server crashes and this is what shows up:

ℹ Compiling Client
ℹ Compiling Server
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] start: razzle start`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     .../.npm/_logs/2021-01-13T05_35_31_851Z-debug.log

[therealgilles]

Coming back to this a bit, I am observing my node server process is not receiving SIGINT when I press ^C to interrupt the process, so it looks like signals are not reaching the node process.

[fivethreeo]

Maybe capture sigint it the plugin process and pass it down. Then not rerun server in plugin if sigint is used, just in case of any race condition.

[therealgilles]

Yes, I've been experimenting with exactly that but I'm not completely satisfied yet. I added this code:

    this.worker = worker;
    // ADD SIGNAL HANDLERS
    ['SIGINT', 'SIGTERM'].forEach(sig => {
      process.once(sig, () => {
        this.worker.kill(sig);
        if (sig === 'SIGINT') this.trigger_exit = true;
      });
    });
    if (callback) callback();

and then:

  _handleChildExit(code, signal) {
    if (this.trigger_exit) { this.worker = null; process.exit(); } // <-- HERE
    if (code) this._error('script exited with code', code);
    if (signal && signal !== 'SIGTERM') this._error('script exited after signal', signal);

which works, but upon exit to terminal, the shell history (if you hit up/down keys) return control characters such as ^[[A and ^[[B. It fixes itself once you hit the return key, but it is a bit annoying.

If I replace process.exit() by a simple return, node start.js keeping running until one hits some key in the terminal and then node exits with the following error:

events.js:291
      throw er; // Unhandled 'error' event
      ^

Error: read EIO
    at TTY.onStreamRead (internal/stream_base_commons.js:209:20)
Emitted 'error' event on ReadStream instance at:
    at emitErrorNT (internal/streams/destroy.js:106:8)
    at emitErrorCloseNT (internal/streams/destroy.js:74:3)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  errno: -5,
  code: 'EIO',
  syscall: 'read'
}

Trying to figure out where this is coming from. Maybe the stdin for the server restarting with 'rs'.

Side notes:

• I see the code is using process.kill(worker.pid). Wondering it's not using worker.kill() instead and if there is a difference.
• I think the client dev server could use a clientDevServer.close() like this:

          // Start Webpack-dev-server
          clientDevServer.listen(port, err => {
            if (err) {
              logger.error(err);
            }
          });
          // ADD SIGNAL HANDLERS
          ['SIGINT', 'SIGTERM'].forEach(sig => {
            process.on(sig, () => {
              clientDevServer.close();
            });
          });
          resolve();

[fivethreeo]

Released v4.0.0-canary.22 now with fixes

[therealgilles]

Thanks, trying it now and it's not quite working for me. The signal var in _handleChildExit always comes as null, so the process never exits. I think it's because I use process.exit() in my server shutdown code for SIGINT.

PS: I don't think you need a return statement after process.exit().

[therealgilles]

In another words, the code is relying on the child process returning a signal on exit, but it may not. This might do:

    if (signal) {
      this._error('script exited after signal', signal);
    }

    if (!signal || signal !== 'SIGTERM') {
      process.exit();
    }

Also wondering if it is safe to use SIGTERM to restart the worker process.

[therealgilles]

With the above code, I can see my server signal handler is working:

  signals:info ... servers stopped by SIGINT +0ms
  signals:info ... gracefully shutting down from SIGINT (Ctrl-C) +0ms
  signals:info Goodbye +0ms
>> ^[[A^[[B

but I get the terminal shell history issue with control characters I was talking about earlier (when this was not happening before adding the SIGINT handler, which is a bit unnerving).

[therealgilles]

Dug into this a bit more. I think this will need to be added to to StartServerPlugin.js:

let childHandlerSet
process.on('SIGINT', () => {
  if (!childHandlerSet) {
    process.stdin.pause();
    process.exit();
  }
});
...
    this.worker = worker; // moved this before setting the _handleWebpackExit handler as it uses this.worker
    process.on('SIGINT', this._handleWebpackExit);
    childHandlerSet = true;
...
    if (signal) {
      this._error('script exited after signal', signal);
    }

    if ((!code || code !== 143) && (!signal || signal !== 'SIGTERM')) {
      process.stdin.pause();
      process.exit();
      return; // probably not necessary
    }

and this should be added to start.js to close both the client and server dev servers:

          ['SIGINT', 'SIGTERM'].forEach(sig => {
            process.on(sig, () => {
              clientDevServer.close();
              watching && watching.close();
            });
          });

I haven't been able to figure out why the shell is not interpreting the control characters right after exit (arrow up/down which is usually shell history). Is that happening for you too?

[fivethreeo]

Any way to not use a global or make it threadsafe somehow? Could you do this as pull requests instead hard to find out what to change.

[therealgilles]

I will see if I can avoid the global. I'm wondering if I'm trying too hard to make SIGINT exit cleanly and as quickly as possible, as this is only for development mode. Clean exit is complex as many parts would need to be waited on: server close, stdout, stdin, child process exit..., and still exit if something gets stuck. Again maybe I'm trying too hard. I guess the most important behavior is someone hits ^C and all the processes stop after a few seconds.

It's still a work in progress, so more of a discussion for now. Will do a PR once it all works as best as possible.

[therealgilles]

The terminal issue may be related to this.

[therealgilles]

I found a workaround for the control characters issue on SIGINT exit by modifying the cmd line in package.json:

    "start": "trap 'echo Detected SIGINT && sleep 2 && echo Goodbye; exit' SIGINT; razzle start",

The 2-second sleep delay is usually long enough for the node processes to send their last messages to stdout/stderr and exit.

Not sure if the underlying issue is related to node or whether this would be useful to include in the package. I'm just happy to have found a solution to this puzzle. I will prepare a PR for the signal handling stuff.

[therealgilles]

Found another possibly related open nodejs issue. I think the problem is that, when the shell is taking back control of the terminal, there are still node messages coming out on the stdout stream, which interfere with the tty. Anyway, unrelated to razzle itself.

[therealgilles]

I should add that the SIGINT trap code workaround for package.json only works properly for npm. yarn calls node itself, so it would need to be called like this from the cmd line:

> sh -c "trap 'echo SIGINT detected... && sleep 2 && echo Goodbye; exit' SIGINT; yarn start"

[therealgilles]

@fivethreeo: #1, #3, #4 have been addressed or fixed.

For #2, do you think there is value in trying to have an option to display compile errors right away? or is it better/sufficient to let the app run and ultimately error out?

[fivethreeo]

Seems like a good idea :)

[therealgilles]

2. I tried to introduce syntax errors in both the client and server files, and the webpack errors are reported. So not sure if something's changed since I encountered this issue, or if my scenario was more complex when I experienced the issue.

Maybe we could close this now and reopen when/if it shows up again.

[fivethreeo]

Yes, closing. If we want to do this change maybe listen to something else than afterEmit.