Skip to content

Conversation

dhadka
Copy link
Contributor

@dhadka dhadka commented May 27, 2021

Due to how uploadChunk is implemented, an error while uploading a chunk causes Promise.all to reject with that error. This then closes the file descriptor inside the finally block. However, if there are any async uploads still running, closing the file descriptor may cause I/O operations to fail with bad file descriptor or similar errors. But most importantly, these errors are uncaught, even though this is all run inside try-catch blocks.

Fixes #154, #441, #467, and actions/toolkit#612

This solution is a bit heavy handed as it just converts all unhandled exceptions to warnings. I also thought about fixing uploadChunk so we don't close the file descriptor until all upload promises complete, but this may cause slowness. Closing the file descriptor kills all uploads and lets the action terminate quickly.

Repro

The following script can reliably reproduce this issue. It essentially create a large file that will take some time to read, then creates two promises: one reads the file and the second causes an error (rejects the promise). The error closes the file descriptor which interrupts the read, producing a "bad file descriptor" error.

import * as fs from 'fs'

process.on('uncaughtException', e => console.log(`${Date.now()} - uncaught exception: ${e}`))
process.on('unhandledRejection', e => console.log(`${Date.now()} - unhandled rejection: ${e}`))

async function run(): Promise<void> {
    fs.writeFileSync("foo.txt", "z".repeat(300000000))

    console.log(`${Date.now()} - opening fd`)
    let fd = fs.openSync("foo.txt", "r")

    try {
        await Promise.all([
            new Promise((resolve, reject) => {
                console.log(`${Date.now()} - start reading`)
                fs.readFile(fd, (e, data) => {
                    console.log(`${Date.now()} - read file callback`)
                    if (e) throw Error(`Rethrowing file error - ${e}`)
                    else resolve("success!")
                })
            }),
            new Promise((resolve, reject) => {
                console.log(`${Date.now()} - rejecting`)
                reject(new Error("something went wrong"))
            })
        ])
    } catch (error) {
        console.log(`${Date.now()} - catching ${error}`)
    } finally {
        console.log(`${Date.now()} - closing fd`)
        fs.closeSync(fd)
    }

    // Keep alive until readFile can fail...
    await new Promise((resolve, reject) => {
        setTimeout(() => resolve("Done!"), 1000)
    })
}

run().catch(e => console.log(`${Date.now()} - post run catch ${e}`))

It doesn't matter if we have try-catch blocks or using run().catch(...), the bad file descriptor error still makes its way out and is only caught by the unhandledException event:

> tsc repro.ts && node repro.js

1622076841093 - opening fd
1622076841109 - start reading
1622076841111 - rejecting
1622076841115 - catching Error: something went wrong
1622076841115 - closing fd
1622076841119 - read file callback
1622076841120 - uncaught exception: Error: Rethrowing file error - Error: EBADF: bad file descriptor, read

@dhadka dhadka requested a review from a team as a code owner May 27, 2021 01:05
@dhadka dhadka requested review from konradpabjan and aiqiaoy May 27, 2021 01:22
@dhadka dhadka merged commit c64c572 into main May 27, 2021
@renovate renovate bot mentioned this pull request May 27, 2021
1 task
bors added a commit to rust-lang/crates.io that referenced this pull request Jul 26, 2021
Update actions/cache action to v2.1.6

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [actions/cache](https://togithub.com/actions/cache) | action | patch | `v2.1.4` -> `v2.1.6` |

---

### Release Notes

<details>
<summary>actions/cache</summary>

### [`v2.1.6`](https://togithub.com/actions/cache/releases/v2.1.6)

[Compare Source](https://togithub.com/actions/cache/compare/v2.1.5...v2.1.6)

-   Catch unhandled "bad file descriptor" errors that sometimes occurs when the cache server returns non-successful response (actions/cache#596)

### [`v2.1.5`](https://togithub.com/actions/cache/releases/v2.1.5)

[Compare Source](https://togithub.com/actions/cache/compare/v2.1.4...v2.1.5)

-   Fix permissions error seen when extracting caches with GNU tar that were previously created using BSD tar (actions/cache#527)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/rust-lang/crates.io).
Copy link

@777666007 777666007 left a comment

Choose a reason for hiding this comment

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

etiennebarrie added a commit to Shopify/maintenance_tasks that referenced this pull request Nov 7, 2022
The error seems similar to actions/cache#596 and
ruby/setup-ruby has actions/cache locked to a version not including that fix.
lawrencewong pushed a commit to lawrencewong/maintenance_tasks that referenced this pull request Apr 29, 2023
The error seems similar to actions/cache#596 and
ruby/setup-ruby has actions/cache locked to a version not including that fix.
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.

Error: EBADF: bad file descriptor, read
3 participants