[ruby-core:104124] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[ruby-core:104124] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[miguelfteixeira]

Issue #17933 has been reported by miguelfteixeira (Miguel Teixeira).

----------------------------------------
Bug #17933: `Net::HTTP#write_timeout` doesn't work with `body_stream`
https://bugs.ruby-lang.org/issues/17933

* Author: miguelfteixeira (Miguel Teixeira)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
----------------------------------------
While testing `Net::HTTP#write_timeout` using the server example from the [Feature Issue](https://bugs.ruby-lang.org/issues/13396), I've noticed that [Faraday](https://github.com/lostisland/faraday) multipart requests (with a file parameter) didn't trigger `WriteTimeout`. The root cause is that [Net::HTTPGenericRequest#send_request_with_body_stream](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http/generic_request.rb#L205-L207) is not using `Net::BufferedIO` (the class that implements the write timeout).

The following patch fixes the issue, but looking at the comments, it's unclear if it will break some existing functionality. Albeit all the tests pass.

```diff
--- a/lib/net/http/generic_request.rb
+++ b/lib/net/http/generic_request.rb
@@ -204,7 +204,7 @@ def send_request_with_body_stream(sock, ver, path, f)
     else
       # copy_stream can sendfile() to sock.io unless we use SSL.
       # If sock.io is an SSLSocket, copy_stream will hit SSL_write()
-      IO.copy_stream(f, sock.io)
+      IO.copy_stream(f, sock)
     end
   end
```

```bash
➜  ruby git:(fix-net-http-write-timeout-body-stream) ✗ ../mspec/bin/mspec run library/net/
ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
[- | ==================100%================== | 00:00:00]      0F      0E

Finished in 2.029623 seconds

187 files, 876 examples, 1214 expectations, 0 failures, 0 errors, 0 tagged
```

Any thoughts on this? It looks like `Net::HTTP#write_timeout` is partially implemented, and we should ensure that it either works on all use cases or an error is raised when it's used in a non-supported use case.

I'm more than happy to help with the implementation. If this patch is the correct approach, I can also create new unit tests to assert the proper behaviour.



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

[ruby-core:104125] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[jean.boussier]

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


> it's unclear if it will break some existing functionality

Well, `copy_stream` can only use its optimizations with real IO objects, so functionally this would still work, but the performance would be heavily degraded. But this optimization is already disabled for SSL sockets, so maybe it doesn't matter as much in practice?

----------------------------------------
Bug #17933: `Net::HTTP#write_timeout` doesn't work with `body_stream`
https://bugs.ruby-lang.org/issues/17933#change-92297

* Author: miguelfteixeira (Miguel Teixeira)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
----------------------------------------
While testing `Net::HTTP#write_timeout` using the server example from the [Feature Issue](https://bugs.ruby-lang.org/issues/13396), I've noticed that [Faraday](https://github.com/lostisland/faraday) multipart requests (with a file parameter) didn't trigger `WriteTimeout`. The root cause is that [Net::HTTPGenericRequest#send_request_with_body_stream](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http/generic_request.rb#L205-L207) is not using `Net::BufferedIO` (the class that implements the write timeout).

The following patch fixes the issue, but looking at the comments, it's unclear if it will break some existing functionality. Albeit all the tests pass.

```diff
--- a/lib/net/http/generic_request.rb
+++ b/lib/net/http/generic_request.rb
@@ -204,7 +204,7 @@ def send_request_with_body_stream(sock, ver, path, f)
     else
       # copy_stream can sendfile() to sock.io unless we use SSL.
       # If sock.io is an SSLSocket, copy_stream will hit SSL_write()
-      IO.copy_stream(f, sock.io)
+      IO.copy_stream(f, sock)
     end
   end
```

```bash
➜  ruby git:(fix-net-http-write-timeout-body-stream) ✗ ../mspec/bin/mspec run library/net/
ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
[- | ==================100%================== | 00:00:00]      0F      0E

Finished in 2.029623 seconds

187 files, 876 examples, 1214 expectations, 0 failures, 0 errors, 0 tagged
```

Any thoughts on this? It looks like `Net::HTTP#write_timeout` is partially implemented, and we should ensure that it either works on all use cases or an error is raised when it's used in a non-supported use case.

I'm more than happy to help with the implementation. If this patch is the correct approach, I can also create new unit tests to assert the proper behaviour.



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

[ruby-core:104126] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[jean.boussier]

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


Previous discussion were `write_timeout` was added and it was noted that `copy_stream` didn't have a timeout: https://bugs.ruby-lang.org/issues/13396

There was talks about the possibility to add it, which everybody agreed it would be best.

----------------------------------------
Bug #17933: `Net::HTTP#write_timeout` doesn't work with `body_stream`
https://bugs.ruby-lang.org/issues/17933#change-92298

* Author: miguelfteixeira (Miguel Teixeira)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
----------------------------------------
While testing `Net::HTTP#write_timeout` using the server example from the [Feature Issue](https://bugs.ruby-lang.org/issues/13396), I've noticed that [Faraday](https://github.com/lostisland/faraday) multipart requests (with a file parameter) didn't trigger `WriteTimeout`. The root cause is that [Net::HTTPGenericRequest#send_request_with_body_stream](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http/generic_request.rb#L205-L207) is not using `Net::BufferedIO` (the class that implements the write timeout).

The following patch fixes the issue, but looking at the comments, it's unclear if it will break some existing functionality. Albeit all the tests pass.

```diff
--- a/lib/net/http/generic_request.rb
+++ b/lib/net/http/generic_request.rb
@@ -204,7 +204,7 @@ def send_request_with_body_stream(sock, ver, path, f)
     else
       # copy_stream can sendfile() to sock.io unless we use SSL.
       # If sock.io is an SSLSocket, copy_stream will hit SSL_write()
-      IO.copy_stream(f, sock.io)
+      IO.copy_stream(f, sock)
     end
   end
```

```bash
➜  ruby git:(fix-net-http-write-timeout-body-stream) ✗ ../mspec/bin/mspec run library/net/
ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
[- | ==================100%================== | 00:00:00]      0F      0E

Finished in 2.029623 seconds

187 files, 876 examples, 1214 expectations, 0 failures, 0 errors, 0 tagged
```

Any thoughts on this? It looks like `Net::HTTP#write_timeout` is partially implemented, and we should ensure that it either works on all use cases or an error is raised when it's used in a non-supported use case.

I'm more than happy to help with the implementation. If this patch is the correct approach, I can also create new unit tests to assert the proper behaviour.



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

[ruby-core:104127] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[miguelfteixeira]

Issue #17933 has been updated by miguelfteixeira (Miguel Teixeira).


byroot (Jean Boussier) wrote in #note-2:
> Previous discussion where `write_timeout` was added and it was noted that `copy_stream` didn't have a timeout: https://bugs.ruby-lang.org/issues/13396
> 
> There was talks about the possibility to add it, which everybody agreed it would be best.

Are you talking about moving the `write_timeout` implementation from `Net::BufferedIO` to `IO.copy_stream`? I was reading the comments and got the idea that it would be another layer of "security", not a replacement.

I may be biased but it looks like the current implementation of [`Net::HTTP` creates a `Net::BufferedIO` instance](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http.rb#L1002) so it would be safer if we always use it when writing to a stream to make it future proof.

----------------------------------------
Bug #17933: `Net::HTTP#write_timeout` doesn't work with `body_stream`
https://bugs.ruby-lang.org/issues/17933#change-92299

* Author: miguelfteixeira (Miguel Teixeira)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
----------------------------------------
While testing `Net::HTTP#write_timeout` using the server example from the [Feature Issue](https://bugs.ruby-lang.org/issues/13396), I've noticed that [Faraday](https://github.com/lostisland/faraday) multipart requests (with a file parameter) didn't trigger `WriteTimeout`. The root cause is that [Net::HTTPGenericRequest#send_request_with_body_stream](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http/generic_request.rb#L205-L207) is not using `Net::BufferedIO` (the class that implements the write timeout).

The following patch fixes the issue, but looking at the comments, it's unclear if it will break some existing functionality. Albeit all the tests pass.

```diff
--- a/lib/net/http/generic_request.rb
+++ b/lib/net/http/generic_request.rb
@@ -204,7 +204,7 @@ def send_request_with_body_stream(sock, ver, path, f)
     else
       # copy_stream can sendfile() to sock.io unless we use SSL.
       # If sock.io is an SSLSocket, copy_stream will hit SSL_write()
-      IO.copy_stream(f, sock.io)
+      IO.copy_stream(f, sock)
     end
   end
```

```bash
➜  ruby git:(fix-net-http-write-timeout-body-stream) ✗ ../mspec/bin/mspec run library/net/
ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
[- | ==================100%================== | 00:00:00]      0F      0E

Finished in 2.029623 seconds

187 files, 876 examples, 1214 expectations, 0 failures, 0 errors, 0 tagged
```

Any thoughts on this? It looks like `Net::HTTP#write_timeout` is partially implemented, and we should ensure that it either works on all use cases or an error is raised when it's used in a non-supported use case.

I'm more than happy to help with the implementation. If this patch is the correct approach, I can also create new unit tests to assert the proper behaviour.



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

[ruby-core:104128] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[jean.boussier]

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


> Are you talking about moving the write_timeout implementation from Net::BufferedIO to IO.copy_stream?

No. `IO.copy_stream` is a low level API that leverage `sendfile(2)` and other similar APIs so that the data transfer happens in the Kernel rather than in user space. For large uploads the performance difference can be quite massive.

However this low-level API has no timeout support. The discussion was about adding timeout support to that so that Net::HTTP could pass a timeout argument.

----------------------------------------
Bug #17933: `Net::HTTP#write_timeout` doesn't work with `body_stream`
https://bugs.ruby-lang.org/issues/17933#change-92300

* Author: miguelfteixeira (Miguel Teixeira)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
----------------------------------------
While testing `Net::HTTP#write_timeout` using the server example from the [Feature Issue](https://bugs.ruby-lang.org/issues/13396), I've noticed that [Faraday](https://github.com/lostisland/faraday) multipart requests (with a file parameter) didn't trigger `WriteTimeout`. The root cause is that [Net::HTTPGenericRequest#send_request_with_body_stream](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http/generic_request.rb#L205-L207) is not using `Net::BufferedIO` (the class that implements the write timeout).

The following patch fixes the issue, but looking at the comments, it's unclear if it will break some existing functionality. Albeit all the tests pass.

```diff
--- a/lib/net/http/generic_request.rb
+++ b/lib/net/http/generic_request.rb
@@ -204,7 +204,7 @@ def send_request_with_body_stream(sock, ver, path, f)
     else
       # copy_stream can sendfile() to sock.io unless we use SSL.
       # If sock.io is an SSLSocket, copy_stream will hit SSL_write()
-      IO.copy_stream(f, sock.io)
+      IO.copy_stream(f, sock)
     end
   end
```

```bash
➜  ruby git:(fix-net-http-write-timeout-body-stream) ✗ ../mspec/bin/mspec run library/net/
ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
[- | ==================100%================== | 00:00:00]      0F      0E

Finished in 2.029623 seconds

187 files, 876 examples, 1214 expectations, 0 failures, 0 errors, 0 tagged
```

Any thoughts on this? It looks like `Net::HTTP#write_timeout` is partially implemented, and we should ensure that it either works on all use cases or an error is raised when it's used in a non-supported use case.

I'm more than happy to help with the implementation. If this patch is the correct approach, I can also create new unit tests to assert the proper behaviour.



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

[ruby-core:104235] [Ruby master Bug#17933] `Net::HTTP#write_timeout` doesn't work with `body_stream`

[miguelfteixeira]

Issue #17933 has been updated by miguelfteixeira (Miguel Teixeira).


I've created a pull request in the [Ruby Github project](https://github.com/ruby/ruby) with the proposed solution: https://github.com/ruby/ruby/pull/4564


----------------------------------------
Bug #17933: `Net::HTTP#write_timeout` doesn't work with `body_stream`
https://bugs.ruby-lang.org/issues/17933#change-92419

* Author: miguelfteixeira (Miguel Teixeira)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
----------------------------------------
While testing `Net::HTTP#write_timeout` using the server example from the [Feature Issue](https://bugs.ruby-lang.org/issues/13396), I've noticed that [Faraday](https://github.com/lostisland/faraday) multipart requests (with a file parameter) didn't trigger `WriteTimeout`. The root cause is that [Net::HTTPGenericRequest#send_request_with_body_stream](https://github.com/ruby/ruby/blob/v2_6_7/lib/net/http/generic_request.rb#L205-L207) is not using `Net::BufferedIO` (the class that implements the write timeout).

The following patch fixes the issue, but looking at the comments, it's unclear if it will break some existing functionality. Albeit all the tests pass.

```diff
--- a/lib/net/http/generic_request.rb
+++ b/lib/net/http/generic_request.rb
@@ -204,7 +204,7 @@ def send_request_with_body_stream(sock, ver, path, f)
     else
       # copy_stream can sendfile() to sock.io unless we use SSL.
       # If sock.io is an SSLSocket, copy_stream will hit SSL_write()
-      IO.copy_stream(f, sock.io)
+      IO.copy_stream(f, sock)
     end
   end
```

```bash
➜  ruby git:(fix-net-http-write-timeout-body-stream) ✗ ../mspec/bin/mspec run library/net/
ruby 2.6.3p62 (2019-04-16 revision 67580) [universal.x86_64-darwin20]
[- | ==================100%================== | 00:00:00]      0F      0E

Finished in 2.029623 seconds

187 files, 876 examples, 1214 expectations, 0 failures, 0 errors, 0 tagged
```

Any thoughts on this? It looks like `Net::HTTP#write_timeout` is partially implemented, and we should ensure that it either works on all use cases or an error is raised when it's used in a non-supported use case.

I'm more than happy to help with the implementation. If this patch is the correct approach, I can also create new unit tests to assert the proper behaviour.



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