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

Support upload from named pipes #748

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

zregvart
Copy link
Contributor

@zregvart zregvart commented Mar 25, 2021

Named pipes report file size as 0, which leads to reading the whole
content into memory (0 is less than 64K). This adds additional check to
make sure that the passed in path is not a named pipe, and in that case
opts for the create-temp-file-to-gzip code path.

When running on GitHub Actions infrastructure on windows node, named
pipes can be created using mkfifo from MSYS2. In that case fs.Statss
isFIFO() returns false, and not true as expected. This case is
detected by process.platform being win32 and the passed file having
length of 0.

As a side note, when MSYS2's mkfifo is run, a pipe file is created:

prw-rw-rw- 1 User None  0 Mar 31 12:58 pipe

If fs.stat is invoked at this point ENOENT error will be thrown. As
soon as the pipe is written to, this pipe file is replaced by two same-
named files:

-rw-r--r-- 1 User None  0 Mar 31 13:00 pipe
-rw-r--r-- 1 User None  0 Mar 31 13:00 pipe

And at this point fs.stat isFIFO() returns false. Even though the
file acts as a named pipe.

@zregvart zregvart requested a review from a team March 25, 2021 20:25
Copy link
Contributor

@konradpabjan konradpabjan left a comment

Choose a reason for hiding this comment

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

Overall these changes look. Thanks for putting out this PR and adding a test! Just one question/concern


// accesses the ReadableStream that was passed into sendStream
// eslint-disable-next-line @typescript-eslint/unbound-method
const stream = mocked(HttpClient.prototype.sendStream).mock.calls[0][2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty cool trick, didn't know about this 👍

@@ -174,6 +179,43 @@ describe('Upload Tests', () => {
expect(uploadResult.uploadSize).toEqual(expectedTotalSize)
})

const onLinuxIt = platform() === 'linux' ? it : it.skip
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there are a reason why this test is only on Linux and are there any platform limitations that can arise? We try to remain platform agnostic as much as possible.

I think it should also be able to at least run on Mac just fine (on Windows named pipes are a considerably different with the \\.\pipe\ file location so I think it's fine to avoid a test for that) but I think you could do something like:

const notWindowsIt = process.platform !== 'win32'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, good point. I think I can do one better, there is also other Unixes where this should work, so testing if mkfifo exists is better. Now there is a test with which mkfifo to see if mkfifo can be found.

@zregvart zregvart force-pushed the pr/named-pipe-upload branch 8 times, most recently from bf1f1c7 to c919018 Compare April 1, 2021 15:45
@konradpabjan
Copy link
Contributor

@zregvart sorry for the delays. Could you sync up with the main branch, CI should go green now (we made some fixes). Afterwards this PR should be able to go in 😃

Named pipes report file size as 0, which leads to reading the whole
content into memory (0 is less than 64K). This adds additional check to
make sure that the passed in path is not a named pipe, and in that case
opts for the create-temp-file-to-gzip code path.

When running on GitHub Actions infrastructure on `windows` node, named
pipes can be created using `mkfifo` from MSYS2. In that case `fs.Stats`s
`isFIFO()` returns `false`, and not `true` as expected. This case is
detected by `process.platform` being `win32` and the passed file having
length of 0.

As a side note, when MSYS2's `mkfifo` is run, a pipe file is created:

```
prw-rw-rw- 1 User None  0 Mar 31 12:58 pipe
```

If `fs.stat` is invoked at this point `ENOENT` error will be thrown. As
soon as the pipe is written to, this pipe file is replaced by two same-
named files:

```
-rw-r--r-- 1 User None  0 Mar 31 13:00 pipe
-rw-r--r-- 1 User None  0 Mar 31 13:00 pipe
```

And at this point `fs.stat` `isFIFO()` returns `false`. Even though the
file acts as a named pipe.
@zregvart
Copy link
Contributor Author

@konradpabjan I've rebased, the workflow run requires approval to run.

// on Windows with mkfifo from MSYS2 stats.isFIFO returns false, so we check if running on Windows node and
// if the file has size of 0 to compensate
const isFIFO =
fileStat.isFIFO() || (process.platform === 'win32' && totalFileSize === 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes look good to me! Would like to get one more pair of eyes on this before merging

@yacaovsnc could you quickly glance over this PR? The only slight concern I have is the (process.platform === 'win32' && totalFileSize === 0) block. I'm not sure if there are any other edge cases where on Windows the file size would be reported as 0 and we don't want to go with an in-memory buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is 👍🏼 . The logic here will make sure any file with size 0 to use the file on disk approach which should be safer compared to the in-memory buffer.

@zregvart
Copy link
Contributor Author

@konradpabjan I'm afraid the Windows support needs to be dropped from this see actions/upload-artifact#281, I can prepare a pull request for that.

@konradpabjan
Copy link
Contributor

Thanks @zregvart

If you could work on a fix for Windows that would be great! I'm currently on vacation and away from my computer but on Monday I can review the changes and release a new version of the package and then sync the changes with upload-artifact.

cc @yacaovsnc for visibility

at-wat pushed a commit to at-wat/actions-toolkit that referenced this pull request Aug 31, 2023
Named pipes report file size as 0, which leads to reading the whole
content into memory (0 is less than 64K). This adds additional check to
make sure that the passed in path is not a named pipe, and in that case
opts for the create-temp-file-to-gzip code path.

When running on GitHub Actions infrastructure on `windows` node, named
pipes can be created using `mkfifo` from MSYS2. In that case `fs.Stats`s
`isFIFO()` returns `false`, and not `true` as expected. This case is
detected by `process.platform` being `win32` and the passed file having
length of 0.

As a side note, when MSYS2's `mkfifo` is run, a pipe file is created:

```
prw-rw-rw- 1 User None  0 Mar 31 12:58 pipe
```

If `fs.stat` is invoked at this point `ENOENT` error will be thrown. As
soon as the pipe is written to, this pipe file is replaced by two same-
named files:

```
-rw-r--r-- 1 User None  0 Mar 31 13:00 pipe
-rw-r--r-- 1 User None  0 Mar 31 13:00 pipe
```

And at this point `fs.stat` `isFIFO()` returns `false`. Even though the
file acts as a named pipe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants