ruby-core@ruby-lang.org archive (unofficial mirror)
 help / color / mirror / Atom feed
* [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
@ 2021-09-27 12:16 byroot (Jean Boussier)
  2021-09-27 16:57 ` [ruby-core:105451] " Eregon (Benoit Daloze)
                   ` (7 more replies)
  0 siblings, 8 replies; 11+ messages in thread
From: byroot (Jean Boussier) @ 2021-09-27 12:16 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been reported by byroot (Jean Boussier).

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105451] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
@ 2021-09-27 16:57 ` Eregon (Benoit Daloze)
  2021-10-01  4:48   ` [ruby-core:105517] " Eric Wong
  2021-09-27 20:11 ` [ruby-core:105453] " ioquatix (Samuel Williams)
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 11+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-09-27 16:57 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by Eregon (Benoit Daloze).


I wonder, can `sendfile(2)` be interrupted by a signal like SIGVTALRM like for read/write?
That might be another strategy to implement a timeout for it.

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93902

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105453] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
  2021-09-27 16:57 ` [ruby-core:105451] " Eregon (Benoit Daloze)
@ 2021-09-27 20:11 ` ioquatix (Samuel Williams)
  2021-10-01  5:01   ` [ruby-core:105518] " Eric Wong
  2021-09-27 21:21 ` [ruby-core:105454] " byroot (Jean Boussier)
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 11+ messages in thread
From: ioquatix (Samuel Williams) @ 2021-09-27 20:11 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by ioquatix (Samuel Williams).


Just FYI: `sendfile` is less flexible and you should generally avoid it. The modern syscall is `splice`. I'll read the rest of the issue in more detail and reply later.

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93903

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105454] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
  2021-09-27 16:57 ` [ruby-core:105451] " Eregon (Benoit Daloze)
  2021-09-27 20:11 ` [ruby-core:105453] " ioquatix (Samuel Williams)
@ 2021-09-27 21:21 ` byroot (Jean Boussier)
  2021-09-28 10:10 ` [ruby-core:105463] " Eregon (Benoit Daloze)
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: byroot (Jean Boussier) @ 2021-09-27 21:21 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by byroot (Jean Boussier).


> I wonder, can sendfile(2) be interrupted by a signal like SIGVTALRM like for read/write?

I think it can? But from my initial research I was under the impression that using ALARM to interrupt system calls was a bit frowned upon and that `poll/select` was generally regarded as preferable.

> sendfile is less flexible and you should generally avoid it. The modern syscall is splice

Indeed. `IO.copy_stream` could use some more modern syscalls like `splice` and `io_uring`. But `sendfile(2)` has the advantage to be relatively portable (older linux, macOS, freeBSD, etc). 

So to me this is a bit orthogonal.

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93904

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105463] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
                   ` (2 preceding siblings ...)
  2021-09-27 21:21 ` [ruby-core:105454] " byroot (Jean Boussier)
@ 2021-09-28 10:10 ` Eregon (Benoit Daloze)
  2021-09-28 10:17 ` [ruby-core:105464] " byroot (Jean Boussier)
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-09-28 10:10 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by Eregon (Benoit Daloze).


byroot (Jean Boussier) wrote in #note-4:
> I think it can? But from my initial research I was under the impression that using ALARM to interrupt system calls was a bit frowned upon and that `poll/select` was generally regarded as preferable.

It's what Ruby already uses to interrupt blocking system calls for Ruby interrupts like signals, Thread#raise, etc (as you might already know), so I think it's fine.
Making an IO non-blocking for IO.copy_stream might have side effects, but maybe they are fine.

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93913

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105464] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
                   ` (3 preceding siblings ...)
  2021-09-28 10:10 ` [ruby-core:105463] " Eregon (Benoit Daloze)
@ 2021-09-28 10:17 ` byroot (Jean Boussier)
  2021-09-30 10:02 ` [ruby-core:105502] " ioquatix (Samuel Williams)
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: byroot (Jean Boussier) @ 2021-09-28 10:17 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by byroot (Jean Boussier).


> It's what Ruby already uses to interrupt blocking system calls for Ruby interrupts like signals, Thread#raise, etc

Thanks for the pointers, I'll have a look.

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93914

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105502] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
                   ` (4 preceding siblings ...)
  2021-09-28 10:17 ` [ruby-core:105464] " byroot (Jean Boussier)
@ 2021-09-30 10:02 ` ioquatix (Samuel Williams)
  2021-09-30 10:09 ` [ruby-core:105503] " byroot (Jean Boussier)
  2021-09-30 20:06 ` [ruby-core:105514] " Eregon (Benoit Daloze)
  7 siblings, 0 replies; 11+ messages in thread
From: ioquatix (Samuel Williams) @ 2021-09-30 10:02 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by ioquatix (Samuel Williams).


> As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

Once a timeout occurs, are you sure we can make such a guarantee about the number of bytes that are copied reliably?

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93951

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105503] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
                   ` (5 preceding siblings ...)
  2021-09-30 10:02 ` [ruby-core:105502] " ioquatix (Samuel Williams)
@ 2021-09-30 10:09 ` byroot (Jean Boussier)
  2021-09-30 20:06 ` [ruby-core:105514] " Eregon (Benoit Daloze)
  7 siblings, 0 replies; 11+ messages in thread
From: byroot (Jean Boussier) @ 2021-09-30 10:09 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by byroot (Jean Boussier).


> Once a timeout occurs, are you sure we can make such a guarantee about the number of bytes that are copied reliably?

Depends. In `sendfile(2)` case, if we use `O_NONBLOCK` then yes we can trust the return value:

> If the transfer was successful, the number of bytes written to
> out_fd is returned.  Note that a successful call to sendfile()
> may write fewer bytes than requested; the caller should be
> prepared to retry the call if there were unsent bytes.

And I believe it's the same for `splice(2)`.

However if we use a signal to interrupt it (assuming it's possible, because the man file doesn't document `EINTR` so I'm not so sure), then the return value would be `-1` so we wouldn't have this information.

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93952

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105514] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
                   ` (6 preceding siblings ...)
  2021-09-30 10:09 ` [ruby-core:105503] " byroot (Jean Boussier)
@ 2021-09-30 20:06 ` Eregon (Benoit Daloze)
  7 siblings, 0 replies; 11+ messages in thread
From: Eregon (Benoit Daloze) @ 2021-09-30 20:06 UTC (permalink / raw)
  To: ruby-core

Issue #18228 has been updated by Eregon (Benoit Daloze).


byroot (Jean Boussier) wrote in #note-8:
> However if we use a signal to interrupt it (assuming it's possible, because the man file doesn't document `EINTR` so I'm not so sure), then the return value would be `-1` so we wouldn't have this information.

I would think it also returns the number of written bytes, otherwise that syscall is completely unusable in the presence of signals (it could infinitely loop with retries).
See write(2) for instance, it returns the number of written bytes:

>        Note that a successful write() may transfer fewer than count bytes.  Such partial writes can occur for various reasons; for exam‐
>       ple, because there was insufficient space on the disk device to write all of the requested bytes, or because a blocked write() to
>       a  socket,  pipe, or similar was interrupted by a signal handler after it had transferred some, but before it had transferred all
>       of the requested bytes. 

----------------------------------------
Feature #18228: Add a `timeout` option to `IO.copy_stream`
https://bugs.ruby-lang.org/issues/18228#change-93961

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
----------------------------------------
### Context

In many situations dealing with large files, `IO.copy_stream` when usable bring major performance gains (often twice faster at the very least). And more importantly, when the copying is deferred to the kernel, the performance is much more consistent as it is less impacted by the CPU utilization on the machine.

However, it is often unsafe to use because it doesn't have a timeout, so you can only use it if both the source and destination IOs are trusted, otherwise it is trivial for an attacker to DOS the service by reading the response very slowly.

### Some examples

- It is [used by `webrick`](https://github.com/ruby/webrick/commit/54be684da9d993ad6c237e2e9853eb98bcbaae6e).
- `Net::HTTP` uses it to send request body if they are IOs, but [it is used with a "fake IO" to allow for timeouts](https://github.com/ruby/net-http/pull/27), so `sendfile(2)` &co are never used.
- [A proof of concept of integrating in puma shows a 2x speedup](https://github.com/puma/puma/pull/2703). 
- [Various other HTTP client could use it as well](https://github.com/nahi/httpclient/pull/383).
- I used it in private projects to download and upload large archives in and out of Google Cloud Storage with great effects.

### Possible implementation

The main difficulty is that the underlying sycalls don't have a timeout either.

The main syscall used in these scenarios is `sendfile(2)`. It doesn't have a timeout parameter, however if called on file descriptors with `O_NONBLOCK` it does return early and allow for a `select/poll` loop. I did a very quick and dirty experiment with this, and it does seem to work.

The other two accelerating syscalls are [`copy_file_range(2)`](https://man7.org/linux/man-pages/man2/copy_file_range.2.html) (linux) and [`fcopyfile(2)`](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fcopyfile.3.html) (macOS). Neither have a timeout, and neither manpage document an `EAGAIN / EWOULDBLOCK` error. However these syscalls are limited to real file copies, generally speaking timeouts for real files are less of a critical need, so it would be possible to simply not use these syscalls if a timeout is provided.

### Interface

`copy_stream(src, dst, copy_length, src_offset, timeout)`
or `copy_stream(src, dst, copy_length, src_offset, timeout: nil)`

As for the return value in case of a timeout, it is important to convey both that a timeout happened, and the number of bytes that were copied, otherwise it makes retries impossible.

- It could simply returns the number of byte, and let the caller compare it to the expected number of bytes copied, but that wouldn't work in cases where the size of `src` isn't known.
- It could return `-1 - bytes_copied`, not particularly elegant but would work.
- It could return multiple values or some kind of result object when a timeout is provided.
- It could raise an error, with `bytes_copied` as an attribute on the error.

Or alternatively `copy_stream` would be left without a timeout, and some kind of `copy_stream2` would be introduced so that `copy_stream` return value wouldn't be made inconsistent.






-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105517] Re: [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 16:57 ` [ruby-core:105451] " Eregon (Benoit Daloze)
@ 2021-10-01  4:48   ` Eric Wong
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-10-01  4:48 UTC (permalink / raw)
  To: ruby-core

"Eregon (Benoit Daloze)" <noreply@ruby-lang.org> wrote:
> I wonder, can `sendfile(2)` be interrupted by a signal like SIGVTALRM like for read/write?
> That might be another strategy to implement a timeout for it.

For the socket/pipe ends, yes (if blocking), that is
interruptible.  Not for regular files (unless it's something
like NFS mounted with interrupts enabled).

> Feature #18228: Add a `timeout` option to `IO.copy_stream`
> https://bugs.ruby-lang.org/issues/18228#change-93902

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [ruby-core:105518] Re: [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream`
  2021-09-27 20:11 ` [ruby-core:105453] " ioquatix (Samuel Williams)
@ 2021-10-01  5:01   ` Eric Wong
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-10-01  5:01 UTC (permalink / raw)
  To: ruby-core

"ioquatix (Samuel Williams)" <noreply@ruby-lang.org> wrote:
> Just FYI: `sendfile` is less flexible and you should generally
> avoid it. The modern syscall is `splice`.

No point in avoiding sendfile, sendfile is considerably easier
to use in common cases and results in fewer syscalls.

splice requires one end to be a pipe; if neither end is a pipe
so you need to create and manage the pipe yourself as an
intermediate buffer.  Basically, instead of:

   void *buf = malloc(...);
   while (read(rfd, buf, ...) > 0)
     write(wfd, buf, ...);
   free(buf);

It becomes:

   int buf[2];
   pipe2(buf, ...);
   while (splice(rfd, ..., buf[1], ...) > 0) /* splice into pipe */
     splice(buf[0], ..., wfd, ...);  /* splice out of pipe */
   close(buf[0]);
   close(buf[1]);

sendfile creates an internal pipe transparently inside the
kernel for doing splice.  The pipe is still there, but private
to the kernel so you won't have to jump between userspace and
kernel space repeatedly.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-10-01  5:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-27 12:16 [ruby-core:105450] [Ruby master Feature#18228] Add a `timeout` option to `IO.copy_stream` byroot (Jean Boussier)
2021-09-27 16:57 ` [ruby-core:105451] " Eregon (Benoit Daloze)
2021-10-01  4:48   ` [ruby-core:105517] " Eric Wong
2021-09-27 20:11 ` [ruby-core:105453] " ioquatix (Samuel Williams)
2021-10-01  5:01   ` [ruby-core:105518] " Eric Wong
2021-09-27 21:21 ` [ruby-core:105454] " byroot (Jean Boussier)
2021-09-28 10:10 ` [ruby-core:105463] " Eregon (Benoit Daloze)
2021-09-28 10:17 ` [ruby-core:105464] " byroot (Jean Boussier)
2021-09-30 10:02 ` [ruby-core:105502] " ioquatix (Samuel Williams)
2021-09-30 10:09 ` [ruby-core:105503] " byroot (Jean Boussier)
2021-09-30 20:06 ` [ruby-core:105514] " Eregon (Benoit Daloze)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).