Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

@juliangruber
Copy link
Member

I don't know why, but the old flow made node crash with code 13, no error message

@juliangruber juliangruber requested a review from bajtos June 20, 2023 16:07
@juliangruber juliangruber merged commit 5ef15fe into main Jun 20, 2023
@juliangruber juliangruber deleted the fix/unzip-windows branch June 20, 2023 16:09
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I have mixed feelings about mixing callback-style events and async functions. IIRC, when the async event listener function fails, there is nobody to handle the rejected promise. Having said that, as long as your solution works, I am fine with it.

@juliangruber
Copy link
Member Author

I don't like it either. Can you suggest a way to improve this?

To me the async function in the event emitter listener is ok, because if it fails it will still make the process crash

@bajtos
Copy link
Member

bajtos commented Jun 21, 2023

To me the async function in the event emitter listener is ok, because if it fails it will still make the process crash

Yeah, makes sense; I think this is actually the simplest option. Maybe we can add a code comment to explicitly explain that we rely on that behaviour for error handling. So that people reading the code know that we are intentionally using this solution for error handling instead of accidentally not considering error handling at all.

A more explicit solution is to wrap everything in a promise, but it's complex and ugly, and not that much better in this particular case where we want to crash the process anyways.

await Promise.all([
  () => new Promise((resolve, reject) => { 
    parser.on('entry', async entry => {
      try {
        const executableFileName = getExecutableFileName(executable)
        const outPath = join(moduleBinaries, module, entry.path)
        await pipeline(entry, createWriteStream(outPath))
        if (entry.path === executableFileName) {
          await chmod(outPath, 0o755)
        }
      } catch (err) {
        reject(err)
      }
    })
    // When to resolve the promise? Does the parser tell us when it's done?
    parser.on('end', () => resolve());
  }),
  pipeline(res.body, parser)
])

Let's keep your current fix 👍🏻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants