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.

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

Implement an async exit package #1

Closed
wants to merge 6 commits into from
Closed

Implement an async exit package #1

wants to merge 6 commits into from

Conversation

jclem
Copy link
Contributor

@jclem jclem commented Apr 22, 2019

Problem: When an action author exits an action via process.exit, the standard out and standard error buffers may still have data yet-to-be-logged in them. This causes logs to be lost, as a user expects to be able to do this:

process.stdout.write('x'.repeat(5e6))
process.exit(0)

Since process.stdout.write is async and process.exit is sync, not all of what's passed to standard out in that script may be logged.

Current solution: An action author can log a special string to standard out that tells the Actions runtime whether the action was successful, neutral, or a failure.

Proposed solution: Add an exit package that users are encouraged to make use of for ending their actions. This package has async functions which drain standard out and standard error before finally exiting. Since users are encouraged to write actions within Toolkit.run, which is async, it is easy to write code that reads much like code that uses process.exit:

Toolkit.run(async tools => {
  const result = await tools.doSomething()

  if (result) {
    await tools.exit.success()
  } else {
    await tools.exit.failure()
  }
})

If an immediate exit is required, the user can pass a sync option (e.g. tools.exit.failure({ sync: true })).

These functions are also exposed independently in the @actions/exit package:

const exit = require('@actions/exit')

process.stdout.write('x'.repeat(5e6))
exit.success() // Technically async, but Node.js will wait for this thread to finish running, anyway

Question: This relies on the bufferSize property from net.Socket (which is a WriteStream). This will only be present if standard out and standard error are TTYs in the Actions runtime (and not, for example, a file).

@TingluoHuang
Copy link
Member

@jclem FYI, AZP doesn't provide TTY for its runtime today.
node -p -e "Boolean(process.stdout.isTTY)" will print out "false" in AZP build today.

@jclem
Copy link
Contributor Author

jclem commented Apr 22, 2019

@jclem FYI, AZP doesn't provide TTY for its runtime today.
node -p -e "Boolean(process.stdout.isTTY)" will print out "false" in AZP build today.

@TingluoHuang What kind of stream would process.stdout be, in that case, in the runtime?

@jclem jclem closed this May 22, 2019
@damccorm damccorm deleted the flush-streams branch August 1, 2019 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants