search

packer-community / winrmcp

Copy files to a remote host using WinRM
MIT License
58 stars 31 forks source link

Re-implemented appendContent to persist a file handle and use `Stream.IO.StreamWriter` for writing. #36

Open arizvisa opened 4 years ago

arizvisa commented 4 years ago

This is a preliminary reimplementation of the appendContent function inside winrmcp/cp.go. This aims to improve performance by reducing the system calls that Powershell will need to make when appending to a file.

When using an i/o redirect, Powershell will need to allocate a handle when opening a file, seek to its end, write the relevant data, and then close the handle. Another negative effect of redirections is that by default it will use UTF-16 which requires twice as many bytes as UTF-8. This PR fixes the deterimental effects of a i/o redirect by using UTF-8, and using the lower-level Stream.IO.StreamWriter class from the .NET framework. This will result in the file handle remaining kept open during the process of the writing of the base64 hunks so that less system calls will need to be made in the hopes that things will be a little faster.

I hate being the guy that uses channels for everything, but I wanted to keep the scope of the variable containing the Stream.IO.StreamWriter within a single function, and a channel seemed the best way to do this. :-/ I believe the same amount of data is being transferred over the socket, however the cost of file writing should be significantly reduced. Maybe it'll have an impact..

This is based on the second suggestion I made in PR #6. It also hasn't been tested too well, and I haven't tested its performance at all as it's just an implementation of a theory. It is also significantly more complex than PR #35 and thus more work could probably be done if you guys believe it should be considered.

russianfool commented 3 years ago

Is this repo all but dead? No commits for two years, hanging pull requests. packer-community in general seems to be low-activity compared to terraform and packer repos.

arizvisa commented 3 years ago

Yeah. Kinda strange. :-/

dylanmei commented 3 years ago

Speaking for myself, and specifically for this project which I was once passionate about, I no longer work with Windows and can't verify changes.

However, I believe the original team would agree with me that stewardship of this GitHub group could be opened up so that good things can still happen here, or else transfer projects to the right place. I'll reach out to them and post in a separate issue, linking back here. Once I've done that, the remainder of this thread can be about this particular PR.

russianfool commented 3 years ago

@dylanmei : No worries, appreciate the response and your contributions :) Didn't mean to derail the pull request, was just the "most-recently-active" thread with no open issue. I came here originally from hashicorp/packer#2648 (main issue for this, but closed there and I didn't want to necro) and this looked more promising than the parallelization in #6.

This may be a semi-moot point anyway as Windows Server 2019 supports OpenSSH. I've yet to test that + terraform/packer (and if remote-exec with ssh connector doesn't work due to command line stuff, local-exec scp should).

dylanmei commented 3 years ago

FYI Seeking maintainers https://github.com/packer-community/winrmcp/issues/38

arizvisa commented 3 years ago

@dylanmei, you also should pin the issue. but don't forget announce it on hashicorp/packer, threatening removal even. packer's the major consumer of this library, so you're likely to find maintainers over there.