This repository has been archived by the owner. It is now read-only.

Close channel after callback executed (request stream ended or no keep alive) #23

Merged
merged 1 commit into from Apr 13, 2016

Conversation

Projects
None yet
3 participants
@hannesm
Member

hannesm commented Apr 9, 2016

Otherwise we end up in situations where the TCP session is closed on the client
side (or no keep-alive is used), but the server still keeps the TCP state around

This is reproducible both on Unix (using tap0) and Xen. It didn't occur too
much more recently since code was copied around which does the close:
https://github.com/mirage/mirage-www/blob/0456a4af2c2902e71c441aab7aeb14b2f0309be5/src/dispatch_tls.ml#L45-L47

And, the TLS close closes the underlying flow (https://github.com/mirleft/ocaml-tls/blob/3428c0f37d4f7df94b371d34307112898c3ad6ea/mirage/tls_mirage.ml#L150-L157)

In general, this should be reworked: each layer should only close its own
protocol state when closing -- it would be good to be able to start a TCP
session, start TLS, data, end TLS, plain data, start another TLS, ...

@avsm @Engil @samoht this fixes Engil/Canopy#14

Close channel after callback executed (request stream ended or no kee…
…p-alive)

Otherwise we end up in situations where the TCP session is closed on the client
side (or no keep-alive is used), but the server still keeps the TCP state around

This is reproducible both on Unix (using tap0) and Xen.  It didn't occur too
much more recently since code was copied around which does the `close`:
https://github.com/mirage/mirage-www/blob/0456a4af2c2902e71c441aab7aeb14b2f0309be5/src/dispatch_tls.ml#L45-L47

And, the TLS close closes the underlying flow (https://github.com/mirleft/ocaml-tls/blob/3428c0f37d4f7df94b371d34307112898c3ad6ea/mirage/tls_mirage.ml#L150-L157)

In general, this should be reworked: each layer should only close its own
protocol state when closing -- it would be good to be able to start a TCP
session, start TLS, data, end TLS, plain data, start another TLS, ...

@hannesm hannesm referenced this pull request Apr 9, 2016

Closed

memory leak #14

@hannesm

This comment has been minimized.

Member

hannesm commented Apr 9, 2016

(it must be in here because afaics cohttp only bothers with read/write of open input/output streams (and errors -- not really properly afaics!?) -- I also couldn't really find any documentation about the expected lifecycle of a flow/channel from the cohttp perspective, maybe someone has more insight))...

in any case, this piece of code creates the channel and thus should close it :)

@hannesm

This comment has been minimized.

Member

hannesm commented Apr 9, 2016

(this also avoids that we run into ocsigen/lwt#222 way too fast)

@talex5

This comment has been minimized.

talex5 commented Apr 9, 2016

BTW, I notice that OCaml 4.03 will warn about leaked channels:

- PR#6902, GPR#210: emit a runtime warning on stderr
  when finalizing an I/O channel which is still open:
    "channel opened on file '...' dies without being closed"
  this is controlled by OCAMLRUNPARAM=W=1 or with Sys.enable_runtime_warnings.
  The behavior of affected program is not changed,
  but they should still be fixed.
  (Alain Frisch, review by Damien Doligez)

Would be good to have this behaviour in Mirage too.

@hannesm

This comment has been minimized.

Member

hannesm commented Apr 9, 2016

@talex5 ah, that's nice. I think some sort of linear types for resource management would be great to have in OCaml/MirageOS (but I'm sure this won't happen in the soon future) -- thus maybe we should just have everybody who allocates a resource without deallocating it within the next 5 lines fill out a paper-form and have it signed and stamped... ;)

@hannesm hannesm referenced this pull request Apr 10, 2016

Closed

exception in accept #9

@talex5

This comment has been minimized.

talex5 commented Apr 10, 2016

Would be nice to use a with_channel here to clean up on success or exception.

Note: although closing things explicitly is correct, I think that whatever created the underlying flow (presumably tcpip) should close it when callback flow returns (or raises). Everything else should then get GC'd automatically at that point, assuming things aren't being stored in globals. How callback's flow gets closed doesn't seem to be documented, though:

module type TCP = sig
  ...
  type callback = flow -> unit io
  (** The type for application callback that receives a [flow] that it
      can read/write to. *)

  val input: t -> listeners:(int -> callback option) -> ipinput
  (** [input t listeners] defines a mapping of threads that are
      willing to accept new flows on a given port.  If the [callback]
      returns [None], the input function will return an RST to refuse
      connections on a port. *)
@hannesm

This comment has been minimized.

Member

hannesm commented Apr 10, 2016

uhm, where is with_channel defined? my approach was to find the least invasive patch for this resource problem, in order to get it merged quickly and have a mirage-http released which is not so resource-hungry.

I think we need to come up with a proposal how to handle lifecycles of resources in MirageOS at some point. And I prefer explicit handling of these, not implicit (leaving it to the finaliser/GC), since this might introduce subtle bugs hard to track down (such as, as you mentioned, if e.g. kept in a global hashtable).

@avsm avsm merged commit c383278 into mirage:master Apr 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hannesm hannesm deleted the hannesm:fix-memleak branch Apr 13, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.