Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56

e-pavlica · 2018-03-02T00:22:44Z

Fixes #50

Changes loader to use child_process.spawn() instead of child_process.execFile()
Adds .jshintrc (to maintain consistent code style)

rhys-vdw · 2018-03-02T00:29:41Z

index.js

-        callback(error, transformedSource, map)
-      }
+      if (config.timeoutMs) { cancelTimeout() }
+      callback(undefined, transformedSource, map)


For consistency please use null as the error value here.

rhys-vdw · 2018-03-02T00:34:38Z

index.js

-      }
+      if (config.timeoutMs) { cancelTimeout() }
+      callback(undefined, transformedSource, map)
+    } else if (child.killed) {


Awesome, I didn't know about child.killed.

rhys-vdw · 2018-03-02T00:48:23Z

index.js

-  )
+  })
+
+  child.stderr.on('data', function (data) {


This will kill the build whenever your Ruby code emits a warning or any logging on stderr, which is not appropriate.

Most likely this should be on the error event.

child.stderr.on('error', callback)

However, note this line from the docs:

Note: The 'exit' event may or may not fire after an error has occurred. When listening to both the 'exit' and 'error' events, it is important to guard against accidentally invoking handler functions multiple times.

So we might consider preventing this with _.once:

var once = require('lodash.once') // ... function transformSource (runner, config, source, map, callback) { var callbackOnce = once(callback) // use only `callbackOnce` below...

There remains a question of what to do with stderr output. I think this should be forwarded to the calling context so that webpack can handle it as normal. This is achievable through the stdio option for spawn.

I believe the correct setting is:

var child = spawn( runner.file, runner.arguments.concat( runnerPath, ioDelimiter, config.engine ), { stdio: ['pipe', 'pipe', process.stderr] } )

This should allow read/write of stdin and stdout via child.stdin and child.stdout respectively. All error logging however will be logged to console during build.

Good call, I hadn't considered warnings in this context. 👍

Looks like we can't listen to stderr with as well as set the stdio option on spawn, but it looks it's behaving as expected by adding { stdio: ['pipe', 'pipe', process.stderr] } and removing the child.stderr.on('error', callback).

Do we expect to see console/warning output from the rails runner? I don't think that behavior is present in the current implementation.

Looks like we can't listen to stderr with as well as set the stdio option on spawn, but it looks it's behaving as expected by adding { stdio: ['pipe', 'pipe', process.stderr] } and removing the child.stderr.on('error', callback).

That sounds correct to me.

Do we expect to see console/warning output from the rails runner? I don't think that behavior is present in the current implementation.

Good question! You're right that we're currently ignoring stderr, but assuming that information being logged to stderr is important it should be surfaced. I think we can pipe it like this and consider adding a flag to suppress it if there are any complaints. My suspicion is that a healthy codebase should not be logging to stderr while the runner is going.

Looks like we can't listen to stderr with as well as set the stdio option on spawn, but it looks it's behaving as expected by adding { stdio: ['pipe', 'pipe', process.stderr] } and removing the child.stderr.on('error', callback).

Ah, I see what you're getting at. My apologies, @xicreative, I actually made a mistake above, the code I meant to show was:

child.on('error', callback)

So that an error in the process fails the build. Note the error event will fire called if:

The process could not be spawned, or

The process could not be killed, or

Sending a message to the child process failed.

None of these are currently handled by the child.on('close' or child.stdin.on('error' I think.

I'm generally unsure whether we should be listening to child.on('close' or child.on('exit'... Do you have a good understanding of consequence of the distinction here? This seems relevant:

Note that when the 'exit' event is triggered, child process stdio streams might still be open.

The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams.

You can see here that execFile does indeed use the 'close' event. This appears to mean that the runner process could end, but stdout and stderr remain open. I'm wondering if this is the underlying cause of #47.

Perhaps the safest course of action is to handle all three child events: 'exit', 'close' and 'error' and respond to the first that fires only.

Thoughts?

@rhys-vdw
As I understand it, the 'close' event relates to the child's io stream, where the 'exit' event relates to the lifecycle of the child process itself. A 'close' can occur after an 'exit', so we probably don't want to act on the 'exit' since we are concerned with capturing all data from the stdout 'data' event before triggering the callback.

We could add an additional timeout on the 'exit' event, in case we're concerned the io stream can get stuck open... I'm not sure if that's a likely, or even a possibility.

Capturing child.on('error') sounds like a great idea.

Ah, it hadn't occurred to me that 'close' would happen after 'exit' unless the stream was forwarded from another process (which I was thinking might have been what spring does). But yes, stick to 'close' because that's what execFile uses. I can play around with isolating the spring problem in a different branch.

rhys-vdw · 2018-03-02T00:49:05Z

index.js

@@ -117,7 +130,7 @@ function transformSource (runner, config, source, map, callback) {
 function addDependencies (loader, paths, callback) {
  var remaining = paths.length

-  if (remaining === 0) callback(null)
+  if (remaining === 0) { callback(null) }


Please remove changes here and respect existing .eslintrc config.

Gosh, I didn't see the .eslintrc! I'll remove the .jshintrc (and these changes). Just didn't want my editor highlighting every line with linting errors while I was trying to work. :-P

rhys-vdw · 2018-03-02T00:49:09Z

index.js

@@ -138,14 +151,14 @@ function addDependencies (loader, paths, callback) {
          )
        }
        remaining--
-        if (remaining === 0) callback(null)
+        if (remaining === 0) { callback(null) }


Please remove changes here and respect existing .eslintrc config.

rhys-vdw · 2018-03-02T00:50:59Z

index.js

      }
    })
  })
 }

 var setTimeoutMsFromTimeoutInPlace = util.deprecate(function (config) {
-  if (config.timeoutMs != null) {
+  if (config.timeoutMs) {


I believe this change is in error because if fails to flag the following ambiguous config:

options: { timeout: 5, timeoutMs: 0 }

rhys-vdw · 2018-03-02T00:52:06Z

test.js

+test('does not error with large files', function (done) {
+  compile2({ file: 'giant.js.erb' }, done, function (stats) {
+    expect(stats.compilation.errors).toEqual([])
+    expect(readOutput()).toMatch(/var bigData = 'a{204740}'/)


Oooh, that's a neat little regex. 👍

rhys-vdw · 2018-03-02T00:53:59Z

test.js

@@ -1,3 +1,5 @@
+/* globals test, expect, fail */


Please remove this comment. There are changes to made here, but we use ESLint. Please see #57.

btw I don't expect you to make these changes! Just letting you know that I'm aware of the issue. Editing that file is pretty terrible in my IDE is pretty terrible right now. If I had time I'd migrate from jest to tape.

rhys-vdw · 2018-03-02T00:59:27Z

Hey @xicreative, really excited to see these changes! Thanks so much for your contribution. I've flagged a bunch of points to address here, let me know if you're happy to make the changes or if you need assistance with anything.

Also please add yourself to the collaborators list in package.json!

I've invited you as a collaborator to the project, which I do as a policy for all of our contributors. It comes with no expectations attached, but I want you to know that you'll always have the ability to make fixes when I'm unavailable.

This reverts commit f650c6a.

e-pavlica · 2018-03-02T21:40:34Z

Thanks @rhys-vdw for the review & invite to collaborate!
The changes you noted have been made. 😃

rhys-vdw · 2018-03-04T23:42:02Z

Hi @xicreative, love your work. Unfortunately I made a mistake in my feedback which appears to have lead to a bit of confusion. Details here.

Let me know what you think, and as always let me know if you need any more help with this. I'm going to do some manual testing of this branch as it stands in UsabilityHub's toolchain.

rhys-vdw · 2018-03-05T02:55:17Z

index.js

-    } else {
+    // When the `runner` command is not found, stdin will not be open.
+    // Attemping to write then causes an EPIPE error. Ignore this because the
+    // `exec` callback gives a more meaningful error that we show to the user.


This comment may be wrong now since we're no longer called exec (in truth it was already stale because we changed to execFile). Either way, if this logic is correct it I suspect it would be the child.on('error' callback that gives the better report.

Looks like the child.on('error') gets an ENOENT if the runner is not found, so we could probably loose this listener entirely. That being said, it doesn't hurt to keep it here either so removing the comment and if statement around the console.error seems like the better solution for future debugging

rhys-vdw · 2018-03-08T00:00:33Z

Looks good! Still a bit unsure about whether we need stdin.on('error') as well as child.on('error'). Also I'd like to catch the ENOENT and display a nicer error message. However this seems like a big step in the right direction. Thanks for your work, and your patience in responding to reviews @xicreative.

rhys-vdw · 2018-03-08T00:18:27Z

5.4.0 released 🎉

e-pavlica added 4 commits March 1, 2018 16:11

Add missing test dependency 'memory-fs'

805589b

Add .jshintrc to help maintain consistent code style

f650c6a

Add test for files that exceed 1 buffer in size

0ecab0e

Change from child_process.execFile to child_file.spawn

9b8eef0

rhys-vdw self-requested a review March 2, 2018 00:39

rhys-vdw requested changes Mar 2, 2018

View reviewed changes

e-pavlica added 5 commits March 2, 2018 10:24

Revert "Add .jshintrc to help maintain consistent code style"

58d0922

This reverts commit f650c6a.

Delegate stderr to process.stderr

2249522

Revert syntax changes

9cdc8ca

Use null for consistency

ccbb4b1

Add myself to contributers

e310ab1

rhys-vdw mentioned this pull request Mar 5, 2018

Module build failed with .vue.erb files #58

Closed

rhys-vdw requested changes Mar 5, 2018

View reviewed changes

Catch error events on runner child; console.error all stdin errors

bbaebdc

rhys-vdw approved these changes Mar 7, 2018

View reviewed changes

rhys-vdw merged commit ac09c2b into usabilityhub:master Mar 8, 2018

dependabot-preview bot mentioned this pull request Mar 18, 2018

Bump rails-erb-loader from 5.3.0 to 5.4.1 in /WcaOnRails thewca/worldcubeassociation.org#2691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56

Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56

e-pavlica commented Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw Mar 2, 2018

e-pavlica Mar 2, 2018

e-pavlica Mar 2, 2018

rhys-vdw Mar 4, 2018

rhys-vdw Mar 4, 2018

e-pavlica Mar 6, 2018

rhys-vdw Mar 7, 2018

rhys-vdw Mar 2, 2018

e-pavlica Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw Mar 2, 2018

rhys-vdw commented Mar 2, 2018

e-pavlica commented Mar 2, 2018

rhys-vdw commented Mar 4, 2018

rhys-vdw Mar 5, 2018

e-pavlica Mar 7, 2018 •

edited

Loading

rhys-vdw commented Mar 8, 2018

rhys-vdw commented Mar 8, 2018 •

edited

Loading

Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56

Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56

Conversation

e-pavlica commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhys-vdw commented Mar 2, 2018

e-pavlica commented Mar 2, 2018

rhys-vdw commented Mar 4, 2018

Choose a reason for hiding this comment

e-pavlica Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

rhys-vdw commented Mar 8, 2018

rhys-vdw commented Mar 8, 2018 • edited Loading

e-pavlica Mar 7, 2018 •

edited

Loading

rhys-vdw commented Mar 8, 2018 •

edited

Loading