Fix for leaked pipes #54
Conversation
| (* Close the pipes we created for ssl when we're done! *) | ||
| don't_wait_for ( | ||
| Pipe.closed net_to_ssl >>= fun () -> | ||
| Deferred.all_ignore [Reader.close app_rd; Writer.close app_wr] |
There was a problem hiding this comment.
We usually specify the order in cases like this. So let's close the writer first.
NB I am not familiar with this codebase so not totally clear if this is the correct fix for this issue
trevorsummerssmith
force-pushed
the
ssl-pipe-leak-fix
branch
from
615b67a
to
365cc94
Compare
|
@rgrinberg I switched the order as per your suggestion. I amended the original commit. |
|
+1. I actually thought |
| @@ -50,4 +50,9 @@ let ssl_listen ~crt_file ~key_file rd wr = | |||
| >>= fun app_rd -> | |||
There was a problem hiding this comment.
Not being that familiar with the code of conduit I'm wondering if it's possible for net_to_ssl to be closed before your cleanup call is made. E.g. after the app_rd is created but before app_wr. Admittedly, that's a little contrived.
There was a problem hiding this comment.
@rgrinberg I don't know either. I am eager to hear from a core committer about this.
There was a problem hiding this comment.
As the core committer, I'm not much more help -- the Async_ssl interface is terribly documented. Am reading the source code now...
There was a problem hiding this comment.
I've tried your patch for 10 minutes with a curl running against cohttp lib_test/test_net_async_server.native, and there's no observable memory leak from a lot of https:// connections with this patch. So merging this...
NB I am not familiar with this codebase so not totally clear if this
is the correct fix for this issue
Fixes #53 for me