-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56
Issues/error stdout maxbuffer exceeded after removing unrelated gem 50 #56
Conversation
index.js
Outdated
callback(error, transformedSource, map) | ||
} | ||
if (config.timeoutMs) { cancelTimeout() } | ||
callback(undefined, transformedSource, map) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency please use null
as the error value here.
index.js
Outdated
} | ||
if (config.timeoutMs) { cancelTimeout() } | ||
callback(undefined, transformedSource, map) | ||
} else if (child.killed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I didn't know about child.killed
.
index.js
Outdated
) | ||
}) | ||
|
||
child.stderr.on('data', function (data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I hadn't considered warnings in this context. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
index.js
Outdated
@@ -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) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove changes here and respect existing .eslintrc config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
index.js
Outdated
@@ -138,14 +151,14 @@ function addDependencies (loader, paths, callback) { | |||
) | |||
} | |||
remaining-- | |||
if (remaining === 0) callback(null) | |||
if (remaining === 0) { callback(null) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove changes here and respect existing .eslintrc config.
index.js
Outdated
} | ||
}) | ||
}) | ||
} | ||
|
||
var setTimeoutMsFromTimeoutInPlace = util.deprecate(function (config) { | ||
if (config.timeoutMs != null) { | ||
if (config.timeoutMs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this change is in error because if fails to flag the following ambiguous config:
options: {
timeout: 5,
timeoutMs: 0
}
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}'/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh, that's a neat little regex. 👍
test.js
Outdated
@@ -1,3 +1,5 @@ | |||
/* globals test, expect, fail */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this comment. There are changes to made here, but we use ESLint. Please see #57.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 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. |
Thanks @rhys-vdw for the review & invite to collaborate! |
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. |
index.js
Outdated
} 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Looks good! Still a bit unsure about whether we need |
5.4.0 released 🎉 |
Fixes #50
child_process.spawn()
instead ofchild_process.execFile()
.jshintrc
(to maintain consistent code style)