Lwt: "Overhaul" Lwt_pool

[aantron]

While keeping the API the same, we should clear up the implementation, and write somewhat better API docs for the interface. Follow-on to #483. cc @hcarty

[bobbypriambodo]

Would you think that this is a proper time to get feedback on Lwt_pool's semantics? I'm thinking of digging into implementations in other libs/languages, but perhaps we also can ask at discuss.ocaml.org (or at least direct them to this issue) for people who use Lwt_pool if they have concerns on the underlying implementation (or missing features). Identify if the current API and behavior makes sense at all, and we can overhaul all the way.

I can't help but feel that the current API of Lwt_pool is kinda strange, but that might just be me. In any case, most of usages of Lwt_pool that I found on GitHub don't specify the optional arguments, and I might have missed it but I found none that uses ?check.

But yeah, tweaking the API or behavior might then introduce major semver update, and I'm not sure if that's acceptable.

[aantron]

Getting feedback on Lwt_pool semantics is indeed good.

Regarding the optional arguments: we probably shouldn't remove them anytime soon anyway. However, since they are optional, we can heavily de-emphasize them, essentially deprecate them, add new optional arguments, or add a new pool creation API that doesn't have them. So there will be a little bit of a legacy annoyance for us to implement for these arguments, but it shouldn't be too bad.

[bobbypriambodo]

Point taken @aantron, thanks. If no one minds I would love to give this a go and see what we can do with it.

[vouillon]

The type of the check argument should probably have been 'a -> bool Lwt.t.

The idea was that if we get an exception while using an element of the pool, the element might not be in a stable state, so we should have the opportunity to put it back in a stable state (for instance, if its a database connection, we may want to abort any transaction that might be in progress) or to dispose it, rather than adding it back to the pool right away.

[bobbypriambodo]

The idea was that if we get an exception while using an element of the pool, the element might not be in a stable state

Yes, that clarifies it. We can use that kind of info in the docs I believe.

[bobbypriambodo]

Okay, so here are my thoughts so far. This is mostly going to be my notes from reading stuff and seeing other's implementations.

A basic, typical resource pool has two public API: acquire and release. Those names should readily imply what they do, so no confusion there. These two functions are the low-level building blocks of a pool.

However, as exposing only these two functions require the user's discipline to call release after acquire to prevent resource leaks, some implementations also provide some kind of a "decorator" or a higher order function, in which the acquired resource is automatically released after it's finished (typically after the function returns). In Lwt_pool, this is what use is; it abstracts away the actual resource acquiring and releasing (by making those two functions private) and will automatically release the resource when the promise resolved.

I'm leaning towards keeping it this way, since it means the API is simpler and users wouldn't have to remember to put the resource back to the pool.

Aside from that, there are several other secondary capabilities of common resource pool implementations:

1. Draining the pool.
2. Validating the pool element before use and disposing if invalid.
3. Validating the pool element after a failed use (e.g. threw exceptions) and disposing if invalid.
4. Having a way to determine whether the pool has available elements or has many blocked waiters.
5. Evicting/recycling old idle pool elements.
6. Having a minimum number of pool elements to load eagerly.
7. Having a way to express whether or not the user want to wait for available element in case of exhausted pool.
8. Determine the order of acquired pool after release (e.g. LIFO or FIFO).

At this point, 1-4 are already supported by Lwt master. There are maybe others that I missed, but just from this list I can see how it can definitely make Lwt_pool go out of hand if we keep adding more features. It's not necessarily a bad thing, but that would mean we intend to make Lwt_pool a feature-complete resource pool, something that I'm not sure Lwt should take responsibility of given that it's a concurrency library (need others' thoughts on this).

So, what to do?

Several things for certain we could do are to improve the docs and polish the API and implementations to clarify the semantics. I think I have a good grasp on how it works now, so I might give it a shot soon.

To be honest, I still think there should be a dedicated resource pool library for OCaml that supports those use cases, and perhaps Lwt_pool can tap into that. But that would mean adding dependency to Lwt. Or perhaps we can extract Lwt_pool out of Lwt. I'm just throwing ideas here :smile: Either way, I believe keeping Lwt_pool as simple as possible is desired.

[aantron]

Great, thanks for this!

On the subject of a dedicated pool library, I think we'd be happy to support that. Lwt_pool doesn't necessarily need to depend on it – we could just add a link to another library from the Lwt_pool docs, something like "if you'd like a more feature-complete resource pool library, try XYZ." It's just one option of many, though.

We could also make sure that Lwt_pool is in a position to support the above features if they are ever requested, document internally what that position is, and wait for the features to be requested. That way, we can be pretty sure that we are avoiding premature work in Lwt (i.e. creating features nobody has a proven use case for), but still be ready to do the work quickly. We would need to add some encouragement to the public docs about submitting issues/PRs on Lwt_pool, to avoid giving users the impression that it's an API that can't be changed.

[bobbypriambodo]

The type of the check argument should probably have been 'a -> bool Lwt.t.

Coming into this again, I think check is superfluous to validate (changing it to the type Jerome suggested will make the types identical). They both serve the same purpose of validating a pool member, one is prior to use and the other after a failed use.

A Github search shows these usages of check:

1. datakit, HARchiver, newque use check to make sure failed promise on use will release/dispose of the element by constantly returning false.
2. ocaml-cassandra actually validate the connection, and drops it if the validation raises.
3. ocaml-stomp exposes that function for clients to use.

I can't comment on number 3. For number 2 It's possible that we can just replace the call to check with validate, and it will behave largely the same (we will perform the same validity checks before use and after failed use).

On number 1, I think we can actually make dropping the pool element on any kind of failure on use without further checking as default (This is what's done in Haskell's Data.Pool), and this change would fulfill use case of number 1 out of the box. I'm aware that in cases where member creation is really expensive, making sure we retain valid elements on exceptions is desired, and if we want to have more control, we can introduce a boolean parameter e.g. drop_on_exception which can then be set to false, hence disposal after failed use will only rely on the validity check through validate.

Yes, these will be breaking changes, so I don't intend to do this soon if at all (perhaps 4.0.0?). But I can see how the API and implementation can be simpler if we go that way. Possibly we can make it gradual by adding ?(drop_on_exception=false) first and make sure that use case number 1 rely on that instead by setting it to true.

Thoughts are appreciated!

[aantron]

All of that seems potentially reasonable. #1 seems the most problematic, as indeed we can't unconditionally drop elements in validate, and it does seem wise not to destroy elements unconditionally when the use promise is rejected.

For a fresh API, the easiest way may be to create a new function use' (with a better name, please :p), and deprecate use. Ideally, we could implement use in a straightforward way over that function, though I am not sure if this will really be possible.

[aantron]

Closing this for now. We can return to it later.