Skip to content
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

Jump to bottom

feat: configurable maxBuffer spawn option #337

Merged
merged 3 commits into from
Mar 13, 2022
Merged

feat: configurable maxBuffer spawn option #337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d219e5
feat: add default spawn properties override: `timeout`, `killSignal` …
antongolub Mar 13, 2022
3558379
refactor: store spawn opts as `$` own values
antongolub Mar 13, 2022
cb2725b
revert: rm spawn timeout option
antongolub Mar 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,8 @@ Or use a CLI argument: `--shell=/bin/bash`
#### `$.spawn`

Specifies a `spawn` api. Defaults to `require('child_process').spawn`.
You may also assign `spawn.timeout`, `spawn.maxBuffer` and `spawn.killSignal`
to override the default spawn options for all calls.

#### `$.prefix`

Expand Down
8 changes: 6 additions & 2 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {ChildProcess, spawn} from 'child_process'
import {ChildProcess, spawn, SpawnOptions} from 'child_process'
import {Readable, Writable} from 'stream'
import * as _fs from 'fs-extra'
import * as _globby from 'globby'
Expand All @@ -30,7 +30,11 @@ interface $ {
shell: string
prefix: string
quote: (input: string) => string
spawn: typeof spawn
spawn: typeof spawn & {
maxBuffer?: number | undefined
timeout?: SpawnOptions['timeout']
killSignal?: SpawnOptions['killSignal']
}
}

export interface ProcessPromise<T> extends Promise<T> {
Expand Down
12 changes: 9 additions & 3 deletions index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ export function registerGlobals() {
}

export function $(pieces, ...args) {
let {verbose, shell, prefix} = $
let {verbose, shell, prefix, spawn} = $
let {timeout, killSignal, maxBuffer = 200 * 1024 * 1024 /* 200 MiB*/} = spawn
antonmedv marked this conversation as resolved.
Show resolved Hide resolved
let __from = (new Error().stack.split(/^\s*at\s/m)[2]).trim()

let cmd = pieces[0], i = 0
Expand All @@ -80,15 +81,20 @@ export function $(pieces, ...args) {
printCmd(cmd)
}

let child = $.spawn(prefix + cmd, {
let child = spawn(prefix + cmd, {
cwd: process.cwd(),
shell: typeof shell === 'string' ? shell : true,
stdio: [promise._inheritStdin ? 'inherit' : 'pipe', 'pipe', 'pipe'],
windowsHide: true,
maxBuffer: 200 * 1024 * 1024, // 200 MiB
maxBuffer,
})

if (timeout) {
promise._timeout = setTimeout(() => promise.kill(killSignal), timeout)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it should go into the library. It's really easy to implement in client code:

let p = $`...`
setTimeout(() => p.kill('SIGINT').catch(_=>_), 1000)
await p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, it's possible to control timeout from a script, but it seems more convenient to hide this logic in API inners just like cp.spawn({timeout: 1000}) does.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let’s think more of zx’s timeout api. In CP timeout is per execution. And here it will be globally.

}

child.on('close', (code, signal) => {
promise._timeout && clearTimeout(promise._timeout)
let message = `${stderr || '\n'} at ${__from}`
message += `\n exit code: ${code}${exitCodeInfo(code) ? ' (' + exitCodeInfo(code) + ')' : ''}`
if (signal !== null) {
Expand Down
14 changes: 14 additions & 0 deletions test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,20 @@ if (test('Retry works')) {
assert(Date.now() >= now + 50 * (5 - 1))
}

if (test('spawn timeout')) {
let signal = 0
$.spawn.timeout = 100
$.spawn.killSignal = 'SIGKILL'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I'm not really like this API. timeout & killSignal seems not related here, but they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://nodejs.org/api/child_process.html#child_processspawncommand-args-options

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the difference between:

$.spawn.timeout = 100
$.spawn.killSignal = 'SIGKILL'

and

{ timeout: 100
  killSignal: 'SIGKILL'}

try {
await $`sleep 9999`
} catch (p) {
signal = p.signal
}
delete $.spawn.timeout
delete $.spawn.killSignal
assert.equal(signal, 'SIGKILL')
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably we can introduce another experimental func (as retry):

await cutoff('SIGKILL', 1000)`...`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's about $.f({...}) factory?

$.f({maxBuffer: 1024 * 1024 * 1024})`cmd arg1 arg2`
//...
$.nothrow = $.f({noThrow: true})
$.quiet = $.f({quiet: true})
$.retry5 = $.f({retry: 5, delay: 1000})
$.timeout1s = $.f({timeout: 1000})
//...
$.custom = $.f({timeout: 10000, maxBuffer: 1024 * 1024 * 1024, retry: 5, delay: 1000, quiet: true, spawn: customSpawn})
// and finally
global.$ = $.custom

or even factory of factories:

$.custom = $.ff({maxBuffer: $.ff.0, timeout: $.ff.1})
$.custom(1024 * 1024, 1000)`cmd arg1 arg2`
$.retry = $.ff({retry: $.ff.0, delay: $.ff.1})
$.retry(5, 1000)`cmd arg1`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe introduce $.options?

Object.assign($.options, {
    quiet: true,
    retry: 5,
    delay: 1000
})

$`cmd args`

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about it too for a while.

What about:

let $$ = quiet(nothrow($))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And store options in IIFE contexts? yep, it might work too.

let version
if (test('require() is working in ESM')) {
let data = require('./package.json')
Expand Down