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

SftpClient: support uploading file from Stream. #271

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Conversation

tmds
Copy link
Owner

@tmds tmds commented Dec 11, 2024

Fixes #267.

cc @yokrysty

@yokrysty
Copy link
Contributor

oh yes i see, seems better you tackled the async writes which i didn't
for my use cases was enough

some questions:
i still don't understand the async usage on streams because you have await previousCopy.ConfigureAwait(false), so everything goes one after another (i also addressed this one in my pull request, but probably you didn't saw it)
i am thinking more like writing chunks at different offsets in parallel

in the method GetPermissionsForFile the Default has an additional & ~PretendUMask
which is skipped when having a Stream and i don't know what is doing, is it needed or not?
in my changes the reason i used the GetPermissionsForFile is because you had there implementation for Windows (which is currently not finished) and Linux

@tmds
Copy link
Owner Author

tmds commented Dec 11, 2024

i still don't understand the async usage on streams because you have await previousCopy.ConfigureAwait(false), so everything goes one after another (i also addressed this one in my pull request, but probably you didn't saw it)
i am thinking more like writing chunks at different offsets in parallel

Here the previousCopy.ConfigureAwait happens at the end, so the writes are happening in parallel (when pipelineSyncWrites).

In the !pipelineSyncWrites case there is some room for optimization.

in the method GetPermissionsForFile the Default has an additional & ~PretendUMask
which is skipped when having a Stream and i don't know what is doing, is it needed or not?

By lack of information GetPermissionsForFile assumes files on Windows aren't writable for 'other'.
Note that when the file/directory is created on the server, it will also apply a umask that filters out permission. Normally, it will filter out the 'other write' permission (and often also the 'group write' permission).

@tmds tmds merged commit 8ed427c into main Dec 11, 2024
2 checks passed
@yokrysty
Copy link
Contributor

still in don't understand how it's not everything messed up at the destination without knowing the offset where to write
but i think because you await CopyToAsync for upload and await WriteAsync for download is doing everything in order

@tmds
Copy link
Owner Author

tmds commented Dec 12, 2024

how it's not everything messed up at the destination

In the !pipelineSyncWrites case, CopyToAsync does serial writing.
In the pipelineSyncWrites case, each CopyBuffer works on a specific offset. The SFTP SSH write operation will write specifically at that offset.

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.

Support uploading from a Stream
2 participants