The Wayback Machine - https://web.archive.org/web/20201201225311/https://github.com/nakabonne/pbgopy/issues/2
Skip to content

/ pbgopy

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

Jump to bottom

Support option to specify buffer size #2

Open
nakabonne opened this issue Nov 29, 2020 · 5 comments
Open

Support option to specify buffer size #2

nakabonne opened this issue Nov 29, 2020 · 5 comments
Labels
enhancement good first issue help wanted

Comments

@nakabonne
Copy link
Owner

@nakabonne nakabonne commented Nov 29, 2020

Currently, it uses ReadAll() to read from stdin, but that's dangerous and it may cause OOM.

Instead, we'd better directly stream it via HTTP. That way doesn't require to buffer at all.

pbgopy/commands/copy.go

Line 49 in 9e86a26

data, err := ioutil.ReadAll(os.Stdin)

@thealamu
Copy link

@thealamu thealamu commented Nov 30, 2020

I would like to work on this but you talk about OOM. Wouldn't that still happen if you keep storing the streamed data in the cache? Or do you mean streaming directly to the "pasting device"? What would a perfect implementation be?
@nakabonne
Copy link
Owner Author

@nakabonne nakabonne commented Nov 30, 2020

@thealamu Sorry for the confusion. It doesn't mean streaming directly to the "pasting device". It means to stream it to the pbgopy server. However, pbgopy supports end-to-end encryption, so I'm beginning to feel it's not appropriate.

iouti.ReadAll() continues to buffer the input from the STDIN until EOF appears. If there is incorrect input, it could eat up the memory on the "copying device". I'm trying to think of something else to do 🤔
@nakabonne nakabonne changed the title Directly stream STDIN via HTTP Support option to specify buffer size Dec 1, 2020
@nakabonne
Copy link
Owner Author

@nakabonne nakabonne commented Dec 1, 2020

Let's change it to fail if you try to copy more data than the specified size with the --size(MB) option. By default 500MB is given.
@thealamu
Copy link

@thealamu thealamu commented Dec 1, 2020

Okay, please assign to me.
@nakabonne
Copy link
Owner Author

@nakabonne nakabonne commented Dec 1, 2020

@thealamu Yes, I'll be happy with that. Your help is essential.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement good first issue help wanted
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@nakabonne @thealamu
You can’t perform that action at this time.