lix-project/lix-install-action
1
0
Fork 0
Code Issues 11 Pull requests Packages Projects Releases Wiki Activity

fix: end the FileStreamWriter for the installer file #47

Merged
colemickens merged 5 commits from colemickens/fix-stream-close into main 2023-10-24 16:22:10 +00:00
Conversation 6 Commits 5 Files changed 6 +534 -561
colemickens commented 2023-10-23 08:52:13 +00:00 (Migrated from github.com)
Copy link
Description

We have a failure on a repo where Error: spawn ETXTBSY was observed after #46 was merged.

Taking a closer look, we should be close()-ing the WritableStream, and then end()ing the FileStreamWriter.

ref:

Checklist
  • Tested changes against a test repository
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • (If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)
##### Description We have a failure on a repo where `Error: spawn ETXTBSY` was observed after #46 was merged. Taking a closer look, we should be ~~`close()`-ing the WritableStream, and then~~ `end()`ing the FileStreamWriter. ref: * ~~`WritableStream`: `close()`: https://nodejs.org/api/webstreams.html#writablestreamclose~~ * `FileStreamWriter`: `end()`: https://nodejs.org/api/stream.html#writableendchunk-encoding-callback ##### Checklist - [ ] Tested changes against a test repository - [ ] Added or updated relevant documentation (leave unchecked if not applicable) - [ ] (If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)
colemickens commented 2023-10-23 09:02:12 +00:00 (Migrated from github.com)
Copy link

Update:

Random:

Update: * `pipeTo()` closes the WritableStream itself (ref: https://2ality.com/2022/06/web-streams-nodejs.html ctrl-f "writablestream is closed") Random: * this seems useful for preventing mistakes of not awaiting awaitables: https://typescript-eslint.io/rules/no-floating-promises/
Hoverbear (Migrated from github.com) reviewed 2023-10-23 14:44:42 +00:00
src/main.ts
@ -438,2 +435,2 @@
const fileStreamWeb = stream.Writable.toWeb(fileStream);
await response.body.pipeTo(fileStreamWeb);
const handle = await open(
tempfile,
Hoverbear (Migrated from github.com) commented 2023-10-23 14:44:41 +00:00
Copy link

Referring to https://nodejs.org/api/fs.html#filehandlecreatewritestreamoptions, I'm not sure this syncs the file. I wonder if we also need to call...
https://nodejs.org/api/fs.html#filehandlesync

Referring to https://nodejs.org/api/fs.html#filehandlecreatewritestreamoptions, I'm not sure this syncs the file. I wonder if we also need to call... https://nodejs.org/api/fs.html#filehandlesync
colemickens (Migrated from github.com) reviewed 2023-10-23 16:13:53 +00:00
src/main.ts
@ -438,2 +435,2 @@
const fileStreamWeb = stream.Writable.toWeb(fileStream);
await response.body.pipeTo(fileStreamWeb);
const handle = await open(
tempfile,
colemickens (Migrated from github.com) commented 2023-10-23 16:13:53 +00:00
Copy link

Ah, yes, and the nice solution I found is to call fs.createWriteStream with options { flush: true}, but of course that's only in node 21.

Ah, yes, and the nice solution I found is to call `fs.createWriteStream` with options `{ flush: true}`, but of course that's only in node 21.
colemickens (Migrated from github.com) reviewed 2023-10-23 16:16:01 +00:00
src/main.ts
@ -438,2 +435,2 @@
const fileStreamWeb = stream.Writable.toWeb(fileStream);
await response.body.pipeTo(fileStreamWeb);
const handle = await open(
tempfile,
colemickens (Migrated from github.com) commented 2023-10-23 16:16:01 +00:00
Copy link

what interesting is that the code I changed wasn't calling sync explicitly, and was just waiting for the finished signal. (which I'd assume is similar to awaiting the pipeTo)

I'm going to going to really wack this with a big hammer, open the file so I get a filehandle, make sure it gets fsync'd explicitly and see how this fares.

what interesting is that the code I changed wasn't calling sync explicitly, and was just waiting for the finished signal. (which I'd assume is similar to awaiting the `pipeTo`) I'm going to going to really wack this with a big hammer, open the file so I get a filehandle, make sure it gets fsync'd explicitly and see how this fares.
colemickens (Migrated from github.com) reviewed 2023-10-23 16:21:50 +00:00
src/main.ts
@ -438,2 +435,2 @@
const fileStreamWeb = stream.Writable.toWeb(fileStream);
await response.body.pipeTo(fileStreamWeb);
const handle = await open(
tempfile,
colemickens (Migrated from github.com) commented 2023-10-23 16:21:50 +00:00
Copy link

further musing: there's a "synchronous append" mode (as) for opening, but not an equivalent for w? But there is rs+:

'rs+': Open file for reading and writing in synchronous mode. Instructs the operating system to bypass the local file system cache.

further musing: there's a "synchronous append" mode (`as`) for opening, but not an equivalent for `w`? But there is `rs+`: > 'rs+': Open file for reading and writing in synchronous mode. Instructs the operating system to bypass the local file system cache.
Hoverbear commented 2023-10-23 18:43:02 +00:00 (Migrated from github.com)
Copy link

Unfortunately this results in an error when running the checks:

nix-installer-action on  colemickens/fix-stream-close [$!] is 📦 v1.0.0 via  v20.8.0 via ❄️  impure (nix-shell-env) took 3s 
❯ npm run all

> nix-installer-action@1.0.0 all
> npm run build && npm run format && npm run lint && npm run package


> nix-installer-action@1.0.0 build
> tsc

src/main.ts:12:25 - error TS2307: Cannot find module 'fetch-retry' or its corresponding type declarations.

12 import fetchRetry_ from "fetch-retry";
                           ~~~~~~~~~~~~~


Found 1 error in src/main.ts:12

I realize now we don't actually note that we use the same style as https://github.com/actions/javascript-action so you need to run npm run all before every commit. :( We need to add this to the readme.

Unfortunately this results in an error when running the checks: ```bash nix-installer-action on  colemickens/fix-stream-close [$!] is 📦 v1.0.0 via  v20.8.0 via ❄️ impure (nix-shell-env) took 3s ❯ npm run all > nix-installer-action@1.0.0 all > npm run build && npm run format && npm run lint && npm run package > nix-installer-action@1.0.0 build > tsc src/main.ts:12:25 - error TS2307: Cannot find module 'fetch-retry' or its corresponding type declarations. 12 import fetchRetry_ from "fetch-retry"; ~~~~~~~~~~~~~ Found 1 error in src/main.ts:12 ``` I realize now we don't actually note that we use the same style as https://github.com/actions/javascript-action so you need to run `npm run all` before every commit. :( We need to add this to the readme.
Hoverbear commented 2023-10-23 18:58:45 +00:00 (Migrated from github.com)
Copy link

#48 should fix the lack of a check!

#48 should fix the lack of a check!
colemickens commented 2023-10-24 08:25:31 +00:00 (Migrated from github.com)
Copy link

I agree we need #48 (and that I had a diff in dist), but the error you got was from not re-running npm install, it definitely in the package-lock.json at the most recent commit.

(edit: also, note the CI was green, so it was able to build, also pointing to it just being a lack of npm install run locally).

I agree we need #48 (and that I had a diff in `dist`), but the error you got was from not re-running `npm install`, it definitely in the `package-lock.json` at the most recent commit. (edit: also, note the CI was green, so it was able to build, also pointing to it just being a lack of `npm install` run locally).
colemickens commented 2023-10-24 08:47:13 +00:00 (Migrated from github.com)
Copy link

I rebased this on #48, hopefully we can merge it first if we want it, otherwise I'll rebase this again.

I rebased this on #48, hopefully we can merge it first if we want it, otherwise I'll rebase this again.
colemickens commented 2023-10-24 09:18:04 +00:00 (Migrated from github.com)
Copy link

now I'm hitting: https://github.com/nodejs/node/issues/48466

now I'm hitting: https://github.com/nodejs/node/issues/48466
Hoverbear (Migrated from github.com) approved these changes 2023-10-24 16:22:01 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Hoverbear
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: lix-project/lix-install-action#47
No description provided.
Delete branch "colemickens/fix-stream-close"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?