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

make sure to close the channel in all cases #24

Merged
merged 1 commit into from Jun 13, 2016

Conversation

Projects
None yet
4 participants
@hannesm
Member

hannesm commented May 5, 2016

(even if a write failed or connection was interrupted)

the underlying stacks will only then free up the resources
(direct: TCP control block (pcb.ml), socket: fd)

(I talked about this in the mirage call yesterday 04-05-2016 15:44 hannes it is leaking memory... guess what I've a new bug for this weekend ;))

make sure to close the channel in all cases
(even if a write failed or connection was interrupted)

the underlying stacks will only then free up the resources
(direct: TCP control block (pcb.ml), socket: fd)
@hannesm

This comment has been minimized.

Member

hannesm commented May 5, 2016

@avsm pls review + merge

@talex5 @samoht another question is whether this should be a Lwt.catch and log the potential failure -- this would allow the interfaces (mirage-net-xen/mirage-net-unix) to assume that their listen callbacks never fail (see discussion in mirage/mirage-net-xen#39 , but also mirage/mirage-tcpip#202 and mirage/mirage-www#448) -- I'm actually not sure how the semantics of receive should be: should the base layer catch the Lwt failure? or should the higher layers only ever return a unit Lwt.t (and never the fail case)?

@talex5

This comment has been minimized.

talex5 commented May 5, 2016

(note: unit Lwt.t includes the possibility of failure - i.e. it means "unit or bug")

Strawman proposal:

  • Whoever calls Lwt.async is responsible for logging uncaught exceptions (i.e. bugs) with some identifying context.
  • If other layers have something useful to add, they can log some context and rethrow. e.g. the HTTP layer can log the requested URL, TCP/IP can log the remote IP and ports, net can log the raw frame.
  • Since exceptions always indicate bugs, we should feel free to log verbose (and maybe expensive) diagnostics.

Alternatively, we could attach the extra context information to the exception and log it all in one place. However, I don't think OCaml provides good support for that.

Perhaps we should move this discussion to the mailing list though?

@hannesm

This comment has been minimized.

Member

hannesm commented May 7, 2016

@talex5 I agree; will try to find some time for writing a mail your proposal does sound good

@avsm would be good to get this merged and released (sorry for the other incomplete fix)

@djs55

This comment has been minimized.

Member

djs55 commented May 7, 2016

Regarding Lwt.async: do you mean that we should have a design rule which says that you must always use a pattern like

Lwt.async (fun () -> Lwt.catch f (fun () -> Log.err (fun f -> f "bad"); Lwt.return ()))

Regarding attaching things to exceptions: I did do something similar once in another life to associate exceptions to backtrace fragments for later reassembly. I used a exn Weak.t and a regular array of backtrace fragments. When I caught an exception I grabbed a new slot (modulo array length, like a closed hash table) and stashed the backtrace fragment in the regular array. If the code wanted to print the backtraces, it would fold over the exn Weak.t selecting slots which contain Some exn for which exn == the one I'm printing. I would then grab the associated backtrace fragments and print them. I got the idea from a blog post but I've lost the reference:

https://github.com/xapi-project/backtrace/blob/06b9fa169d2480373d91fca607280e82e89fbdfa/lib/backtrace.ml#L56

It seemed to work ok-ish, although in this case you had to remember to stash the fragments before erasing them by raising new exceptions, and it tended to create overlapping fragments which I didn't have any code for dedup'ing. I always thought of it like that "shotgun" genome sequencing technique, where the sequencing is done on fragments massively in parallel and the results carefully merged together... just without the careful merging ;)

@talex5

This comment has been minimized.

talex5 commented May 8, 2016

@djs55 yes, that's what I was thinking (except that it should include the actual exception, of course). Then instead of Not_found, you'd get something like [net-xen:frontend] Error processing frame: Not_found, and previous entries in the log might provide more information.

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 12, 2016

..no activity since > 4 weeks for this trivial bugfix... any chance someone maintains this repo (@avsm)?

@samoht samoht merged commit 3fee980 into mirage:master Jun 13, 2016

@samoht

This comment has been minimized.

Member

samoht commented Jun 13, 2016

Thanks! I'll make a new release.

@hannesm hannesm deleted the hannesm:fix-resource-leakage branch Jun 13, 2016

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