H2: shutdown the connection once finished #23
Conversation
src/http_lwt_client.ml
Outdated
| @@ -268,7 +267,9 @@ let single_h2_request ?config fd scheme user_pass host meth path headers body f | |||
| | Some body -> H2.Body.Writer.write_string request_body body | |||
| | None -> ()); | |||
| H2.Body.Writer.close request_body; | |||
| finished | |||
| finished >|= fun res -> | |||
There was a problem hiding this comment.
We're using the Lwt_result.Infix operators in this module -- so I wonder whether the call to shutdown below should be done in all cases (not only the successful ones)?
There was a problem hiding this comment.
I pushed a commit that handles if finished is somehow cancelled using Lwt.finalize. Otherwise finished returns a result so I think it handles failures already? What do you have in mind?
There was a problem hiding this comment.
Looks fine, the alternative would be:
Lwt.bind finished (fun r -> H2.Client_connection.shutdown connection; Lwt.return r)I suspect that finalize is preferably - and still suspect that "cancelling http_lwt_client request" may lead to resource leaks... (but that's fine since it was the case before)
|
Nice find. And indeed, in http-mirage-client we call the shutdown.. https://git.robur.coop/robur/opam-mirror/commit/d03cd65dcdbe99d230cac84f2100f74b427a8675 |
"Funny" how I arrived at the same solution :) |
CHANGES: * Call shutdown in h2 to properly close http2 connections. This fixes a file descriptor leak reported in robur-coop/http-lwt-client#22 (robur-coop/http-lwt-client#23, @reynir @hannesm)
CHANGES: * Call shutdown in h2 to properly close http2 connections. This fixes a file descriptor leak reported in robur-coop/http-lwt-client#22 (robur-coop/http-lwt-client#23, @reynir @hannesm)
CHANGES: * http2: shutdown the connection once finished (robur-coop/http-lwt-client#23, @reynir) * update to TLS 1.0 API (robur-coop/http-lwt-client#25, @hannesm)
CHANGES: * http2: shutdown the connection once finished (robur-coop/http-lwt-client#23, @reynir) * update to TLS 1.0 API (robur-coop/http-lwt-client#25, @hannesm)
The branch https://github.com/reynir/http-lwt-client/tree/test-fd-leak contains a test that reproduces this behavior. It is not included here as it makes some quite brittle assumptions over file descriptor layout.
Closes #22