Lwt: Request: function to clear/destroy Lwt_pool

[bobbypriambodo]

183 already asked for this but was closed, so creating a new issue: will we be able to have a function to clear the pool?

My use case: I'm maintaining a pool of postgresql-ocaml connections in my app. It's a wrapper of libpq, the C client library to postgres. The best usage practice of this library is that we must call a finish function on a connection when it's no longer needed to free the allocated memory. I want to be able to call finish on all connections in the pool when my app shuts down to make sure all connections are properly closed.

Through validate I am only able to check if a connection is bad which will then be reconnected.

An API that I have in mind is by providing an optional ?destroy:('a -> unit Lwt.t) argument to Lwt_pool.create and the public API of Lwt_pool.clear pool which will call destroy on all pooled resource. Is it possible? Would love to give it a shot.

(Btw, Lwt_pool is awesome, seems like so far it's the only public implementation of resource pool in OCaml?)

[aantron]

@bobbypriambodo Since ?destroy interacts only with clear, it seems like adding "extra" functionality to pools that does not actually interact with the main functionality that pools provide (though it may be convenient). It seems that you should be able to track your connections separately, and separately call a suitable destroy function on all of them.

I'm not actually against adding this, I'd just like to have a conversation about how we decide what belongs in the API of Lwt_pool and what doesn't, and whether we should go for a minimalistic and strongly-factored design, or a more expansive "full-service" one. Whatever we decide, we should probably update the manual, too, so it's clear to readers how Lwt_pool fits into a bigger program that uses it.

The optional arguments that are there now seem to provide functionality that more or less can't be implemented externally to Lwt_pool.

@hcarty, any thoughts?

[hcarty]

At a minimum we should document how to handle this situation. #183 hints that it's something I've run into before! It's been unclear what, if any, successful approaches others use for the same kind of issue.

I think a good first step would be to create a working example which uses the external state approach. I don't have time spend on this now unfortunately. I have some work coming up which may overlap though so if no one else gets to it first I'll try to extract an example from my project.

I like the idea of keeping Lwt_pool relatively minimal. I also like the convenience of a function like clear. Ideally we'll show that a function like clear is general enough that it's useful during the normal operation of a program before adding it.

[bobbypriambodo]

I'm not actually against adding this, I'd just like to have a conversation about how we decide what belongs in the API of Lwt_pool and what doesn't

Sure! I don't mind. My rationale is that I think the ability to destroy a resource + clear a pool seems to be quite a common feature in resource pools as seen in Node.js, Haskell, Java, Scala, Clojure, and Erlang/Elixir (ones I managed to Google in a jiffy). It also seems to me that the common characteristics of a "resource" (connections, files, etc.) include that it needs to be freed after used.

It seems that you should be able to track your connections separately, and separately call a suitable destroy function on all of them.

I think I could, but then I would have two "pools" to manage...? E.g. how would I keep them synced when Lwt_pool recreates connections? Also I would think that destroy should be called when the resource is recreated through validate to make sure that the resource is properly destroyed (therefore freeing memory and such). Would love your input if I missed something.

[hcarty]

I like @bobbypriambodo's latest suggestion because it would make validate and check logic simpler (the function doesn't have to dispose of invalid pool elements) and destroy could be used for the requested clear function.

[aantron]

how would I keep them synced when Lwt_pool recreates connections?

Since you pass f to the pool, you would write the logic in f for what needs to be triggered/what book-keeping needs to be done on creation. I don't think validate destroys resources, it just checks for already-destroyed resources, but if your resources can become invalid before being destroyed, then the callback you pass as ?validate does have access to the resource for you to destroy it. Likewise, the callback ?check gets passed the resource as well, so you can destroy it there. I don't think there is any other point where Lwt_pool can decide that a resource is unused. It is true that you can't force-clear an Lwt_pool, except maybe by calling use in a loop and destroying and marking invalid everything in your ?validate callback, until you're out of resources.

I don't think this is a particularly easy-to-understand API, I just had to think about it a lot while writing this comment. I personally would be happy with a redesign (or renaming?). Having a ?destroy as a primitive callback is much more likely to be intuitive.

The names ?check and ?validate are also essentially synonyms, so it's weird that they do different things. It's hard to remember what the difference is.

The pools you linked to do look nicer than Lwt_pool.

[bobbypriambodo]

The names ?check and ?validate are also essentially synonyms, so it's weird that they do different things. It's hard to remember what the difference is.

Actually to this moment I'm still not sure what those two does, the relationship between the two, and how they are intended to be used 😄 docs is almost positive to be an issue. From the docs:

The optional function check is called after a use of an element failed

How is a use of an element failed? Due to validate returning false?

It must call its argument exactly once with true if the element is still valid and false otherwise.

I'm honestly kind of lost here. Isn't validate the one responsible for checking whether the element is still valid? An example might be helpful.

but if your resources can become invalid before being destroyed, then the callback you pass as ?validate does have access to the resource for you to destroy it. Likewise, the callback ?check gets passed the resource as well, so you can destroy it there.

This might be because I don't understand the use of those functions, but it sounds really unpredictable. As much as OCaml allows to intermingle side effects, I would be surprised if the validate or check (by the name alone) functions would do some side effect to the resource.

[aantron]

Well validate and check are provided by the user, so yes they can do anything – that's the intent. Otherwise, I agree with everything.

[bobbypriambodo]

I am not really sure right now since I come across Ocsimore's ocsi_sql module which also uses Lwt_pool to maintain a pool of DB connections (in this case it's using PGOCaml), and even when PGOCaml provides a PGOCaml.close function, this file does not seem to call it.

Perhaps closing DB connections are fine to be ignored? Although I think that would be a source of nasty memory leaks...

[aantron]

@bobbypriambodo I don't have enough expertise to properly answer this for all cases, but considering the specific use case you described, as a well as a "typical" "simple" usage pattern of a DB connection, the DB connection is closed only when the process that opened it is exiting anyway. Given that DB operations tend to be implemented as transactions, I would expect there to be nothing to be "flushed" or otherwise performed on a close that is already otherwise clean from the app's point of view. That leaves only open fds, sockets, and app-side allocated memory, but all those are closed automatically and implicitly deallocated by process exit. If the database is provided by a daemon/server, it will see the implicit close on its end and react appropriately. In the case of a database on a filesystem (e.g. sqlite), there is presumably nothing else to do.

Of course, that doesn't apply in case of deliberate closing of connections by the app when it is not exiting or shutting down. That is where someone with more expertise needs to come in :)

[hcarty]

I'm going to hack on this to try adding an optional dispose argument. How does this interface + doc comment look?

val create :
  int ->
  ?check : ('a -> (bool -> unit) -> unit) ->
  ?validate : ('a -> bool Lwt.t) ->
  ?dispose : ('a -> unit Lwt.t) ->
  (unit -> 'a Lwt.t) -> 'a t
  (** [create n ?check ?validate ?dispose f] creates a new pool with at most
      [n] elements. [f] is the function to use to create a new element.
      Elements are created on demand.

      An element of the pool is validated by the optional [validate] function
      before it is accessed by {!use}. Invalid elements are passed to [dispose]
      and then re-created with [f].

      If a call to {!use} fails with a pool element that element will be passed
      to the optional function [check] as [check element callback].  [check]
      must call [callback] exactly once with [true] if [element] is still valid
      and [false] otherwise.  If [check] calls [callback false] then [dispose]
      will be run on [element]. *)

[bobbypriambodo]

Neat! @hcarty I left some comments on the PR.