Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On the type of Luv.Loop.run and "natural APIs" more generally. #54

Open
glennsl opened this issue Mar 29, 2020 · 9 comments
Open

On the type of Luv.Loop.run and "natural APIs" more generally. #54

glennsl opened this issue Mar 29, 2020 · 9 comments
Labels

Comments

@glennsl
Copy link
Contributor

glennsl commented Mar 29, 2020

Luv.Loop.run returns a boolean value whose meaning does not seem to be documented and, judging by the examples, in most cases is not used, but usually just ignored.

I would argue that using ignore without any type annotation is very error-prone because it sets the trap for accidental partial application. It should be considered bad style IMO, and therefore should be avoided particularly in examples. I also think Luv.Loop.run exacerbates the likelihood of errors because its arguments are all optional except for the terminating unit. It's very easy to specify one or more optional arguments but forget the terminating unit. This leads to partial application, and with the use of ignore will go entirely unnoticed except for the observation that nothing is happening, which is a symptom that doesn't narrow down the possible causes a whole lot.

These issues are also in most cases preventable by providing a better API, for example:

val run_indefinitely : ?loop:t -> unit -> unit
val run_once : ?loop:t -> unit -> bool
val run_once_wait : ?loop:t -> unit -> bool

I still don't know what the bool means of course, so this might still be way off. My assumption here is that it means there's more work to do, or something along those lines. But if it means something else this API probably doesn't make much sense.

Furthermore, it's a stated goal for Luv to provide "more natural APIs":

where libuv is forced to offer difficult APIs due to the limitations of C, Luv provides more natural APIs

But it's also stated that Luv is "a fairly thin binding", which to a significant extent opposes the above goal. And there's certainly also a documentation and familiarity benefit of having the API closely mirroring the original.

So there are two parts to this issue I think. One is the more general question of how much deviation from the C API is considered acceptable. Where the line is, or should be, drawn between providing "natural APIs" and being a thin binding. The other is the specific question concerning this particular API, its use cases, the meaning of the boolean and what follows as a natural API

To summarize:

  1. The bool return type of Luv.Loop.run is not documented.
  2. It is apparently typically not used, which leads to prevalent use of ignore.
  3. ignore is error-prone and IMO bad style.
  4. It can be prevented by providing a better, more natural, API (in some future version 2.0).
  5. But the main question is whether doing so aligns with Luv's goals.

PS. I'm just starting to get my feet wet with Luv, and so far it seems very solid and well-documented, although in need of some polish in places. I ask the more general question because this isn't the only part of the API that feels unnatural. The use of init everywhere, for example, seems unnecessary in most use cases. Anyway, it's certainly still very usable thanks to the great work you've done on documentation, and I want to emphasize how much I personally appreciate that little detail, as well as thank you for the library as a whole.

@aantron
Copy link
Owner

aantron commented Mar 29, 2020

Here's an initial "thinking" response. None of it reflects any "final" decisions I am making; instead, I just want to reply with my thoughts until now, such as the considerations that went into making the OCaml API how it is. We can discuss further :)

Luv.Loop.run returns a boolean value whose meaning does not seem to be documented and, judging by the examples, in most cases is not used, but usually just ignored.

Luv.Loop.run's return value is documented at uv_run in the libuv docs, in each of the bullet points of the list. In general, the OCaml docs attempt to be thin, provide links to the C docs, and treat the C docs as the definitive reference. This is true not only with respect to libuv, but especially more so with respect to linking man pages, where there is no hope, for example, of describing all the error conditions exactly, since they differ system by system. For this reason, the docs also link to at least POSIX man pages wherever possible, and the intent is for readers to visit them for a full understanding.

Would it help to more prominently indicate to readers, that the docs should be used in this way?

I would argue that using ignore without any type annotation is very error-prone because it sets the trap for accidental partial application. It should be considered bad style IMO, and therefore should be avoided particularly in examples. I also think Luv.Loop.run exacerbates the likelihood of errors because its arguments are all optional except for the terminating unit. It's very easy to specify one or more optional arguments but forget the terminating unit. This leads to partial application, and with the use of ignore will go entirely unnoticed except for the observation that nothing is happening, which is a symptom that doesn't narrow down the possible causes a whole lot.

This is definitely a real ergonomics issue on the OCaml side. I should probably begin with rewriting the examples not to use ignore. After that, we can look into a higher-level API.

Furthermore, it's a stated goal for Luv to provide "more natural APIs":

where libuv is forced to offer difficult APIs due to the limitations of C, Luv provides more natural APIs

But it's also stated that Luv is "a fairly thin binding", which to a significant extent opposes the above goal. And there's certainly also a documentation and familiarity benefit of having the API closely mirroring the original.

Indeed, but the stated goal is not exactly to provide "more natural APIs" — quoting it that way discards context, and if that context is not discarded, the opposition in goals goes away, too :) As it says, the goal of more natural APIs only applies in places where the libuv API is hampered by C, such as in uv_spawn, where the huge number of options leads libuv to choose to take a struct as an argument, with several helper enums, etc. In all other cases, the goal is to keep the API as easy to map to the C API as possible. So, the "more natural APIs" goal only applies where the C API is so thoroughly annoying, that you'd rather not learn it in the first place, and don't want anything to correspond directly to it, but just want a mapping from a more natural API explained in the OCaml docs, in case you need to refer to C :) In other words, we can readily imagine that users and authors of uv_spawn didn't want it to be the way it is, but they had no better choice.

So there are two parts to this issue I think. One is the more general question of how much deviation from the C API is considered acceptable. Where the line is, or should be, drawn between providing "natural APIs" and being a thin binding. The other [...]

I agree with this. Part of the issue is that I originally saw two "major" use cases for Luv:

  1. Internally in Lwt_unix, to replace the current implementation by libuv.
  2. Internally in a wrapper that will drive libuv (through Luv) on native, and Node on JS.

Since I imagined the library would be mostly used in the implementation of other libraries, I thought it's acceptable (even desirable) to trade polish of the API for direct, predictable correspondence with the behavior of libuv, so that Luv stays out of the way of higher-level implementors as much as possible, except where all likely implementors would probably agree that something should just be taken care of as part of the translation done by Luv.

If Luv gets more direct usage, this will probably have to be adjusted in some way. Perhaps a thin wrapper with less sharp corners would need to be written, or there is a compromise on all strange APIs that can be found, that makes Luv directly suitable for both kinds of usage.

@aantron aantron added the docs label Mar 29, 2020
@aantron
Copy link
Owner

aantron commented Mar 29, 2020

Also

  1. It can be prevented by providing a better, more natural, API (in some future version 2.0).

We can just break Luv all the time, it's in 0.x and has few users :)

The use of init everywhere, for example, seems unnecessary in most use cases

Don't we need creation functions for most kinds of objects?

The choice of name init is not typical for OCaml, but I made it to correspond to the names in the libuv API.

Also, I will add a sentence at Luv.Loop.run about looking in uv_run docs for the explanation of the return value.

aantron added a commit that referenced this issue Mar 29, 2020
@aantron aantron removed the docs label Mar 29, 2020
@aantron
Copy link
Owner

aantron commented Mar 29, 2020

I added the minimal, initial change of replacing

ignore (Luv.Loop.run ())

by

ignore (Luv.Loop.run () : bool)

in the README and all examples, so that at least it forms a habit that, if practiced, will make partial application less likely. Posted the new docs.

@glennsl
Copy link
Contributor Author

glennsl commented Mar 29, 2020

Thanks! I completely agree with the approach of having Luv be thin low-level bindings that correspond closely with its C counterpart, and building higher-level bindings on top of that. I've even used this approach before myself. I guess my expectations were just skewed a bit because despite being low-level and relatively unpolished, this is still the most polished and well-documented API I'm aware of for what it provides!

It might still make sense to make minor alterations like turning the Loop modes into separate functions, but only as long as the name reflects that they're all variations of uv_run. I think the API I proposed above does that, despite having other issues.

Would it help to more prominently indicate to readers, that the docs should be used in this way?

Definitely. Although I did look at the docs for uv_run, but I guess I expected a stand-alone sentence describing the return type and didn't consider that it would depend so much on the "mode". There's still a difference between the int returned by uv_run and the bool returned by Loop.run though, and the relation between them isn't entirely obvious. I'm guessing 0 here corresponds to false, but often C APIs return error codes where 0 means success, which might more naturally map to true.

[...] quoting it that way discards context, and if that context is not discarded, the opposition in goals goes away, too :)

To be fair, I did quote it with context too, although that still didn't stop me from interpreting it wider than intended :)

Don't we need creation functions for most kinds of objects?

The choice of name init is not typical for OCaml, but I made it to correspond to the names in the libuv API.

The APIs I've interacted with so far, mostly just Loop and FS_event, don't seem like objects though. In FS_event, for example, it would in most cases be sufficient for start and init to be a single function, returning a handle to be passed to stop later. I can't imagine a scenario where it makes sense to call init without immediately calling start.

Of course arguments to the contrary include consistency across modules and correspondence with the C API, and I think you've already convinced me that it should remain as it is. But if the goal was a more natural API then init certainly sticks out in this case at least.

aantron added a commit that referenced this issue Mar 29, 2020
@aantron
Copy link
Owner

aantron commented Mar 29, 2020

It might still make sense to make minor alterations like turning the Loop modes into separate functions, but only as long as the name reflects that they're all variations of uv_run. I think the API I proposed above does that, despite having other issues.

It occurs to me that we probably only need one of those, Luv.Loop.run_indefinitely. The other modes are kind of "advanced." They would probably be only used for people working on an integration or doing something unusual, i.e. experts, and I think we can realistically rely on them looking at the function signature of run carefully.

I expected a stand-alone sentence describing the return type and didn't consider that it would depend so much on the "mode". There's still a difference between the int returned by uv_run and the bool returned by Loop.run though, and the relation between them isn't entirely obvious. I'm guessing 0 here corresponds to false, but often C APIs return error codes where 0 means success, which might more naturally map to true.

Thanks, I added a further commit with a sentence about that.

The APIs I've interacted with so far, mostly just Loop and FS_event, don't seem like objects though.

Indeed, FS_event would look a bit different in an OCaml API. But it is an object in libuv, because it is an FS_event handle, and has all the handle functions, etc. A higher-level wrapper of libuv that did filesystem watching would probably hide all that (it's been discussed :)).

Apart from consistency on the level of aesthetics or reading docs, there is a certain subtype relationship between libuv base handles and FS_event that I'd like the basic Luv API to maintain. For example, among other things, active FS_event handles can be found through loop querying functions. I suspect this kind of usage of a loop and an FS_event handle would be extremely rare, and limited to experts only. Nonetheless I wouldn't want Luv itself to become a technical obstacle to it.

aantron added a commit that referenced this issue Mar 29, 2020
@aantron aantron added the docs label Mar 29, 2020
@aantron aantron added this to the 0.5.2 milestone Mar 29, 2020
@glennsl
Copy link
Contributor Author

glennsl commented Mar 29, 2020

It occurs to me that we probably only need one of those, Luv.Loop.run_indefinitely. The other modes are kind of "advanced." They would probably be only used for people working on an integration or doing something unusual, i.e. experts, and I think we can realistically rely on them looking at the function signature of run carefully.

Yep, like me 😬 So maybe not 😉

I'm integrating with Revery, which has its own app loop and offers no control over it. I have to hook into it with what's essentially a setTimeout(..., 0) and run the loop with `NOWAIT.
Revery in turn (via another dependency) uses requestAnimationFrame when compiled to JS, so I don't think it can be made to offer much more control over the loop either. I think this scenario might be relatively common as well.

A higher-level wrapper of libuv that did filesystem watching would probably hide all that (it's been discussed :)).

Cool! We need an API that does globbing as well, or essentially the ability to emulate the fswatch API. So if that aligns with those plans, I'm pretty sure we can offer some manpower to get it going relatively soon.

Apart from consistency on the level of aesthetics or reading docs, there is a certain subtype relationship between libuv base handles and FS_event that I'd like the basic Luv API to maintain. [...] I wouldn't want Luv itself to become a technical obstacle to it.

Yeah that makes complete sense. Thanks for explaining!

@aantron aantron removed this from the 0.5.x milestone Jan 14, 2021
@joprice
Copy link

joprice commented Aug 21, 2021

Related question that maybe could result in something being added to the docs: When you are using the default mode, and you get true back, that corresponds to non-zero value from libuv, so that indicates that there are still active handles or requests. Should this be treated as an error case by the caller, and other calls should be made first to close connections, or is this expected? Does it just indicate that at the time of calling Luv.Handle.close, there were active connections, but they will all be closed cleanly?

@aantron
Copy link
Owner

aantron commented Aug 22, 2021

Could you create a little program that creates a long-lived connection to some test server, then stops the default loop. Does this cause run to return true? If so, do you observe the connection getting closed, or does it linger on? I will add a note to the docs with your findings.

@aantron
Copy link
Owner

aantron commented Aug 22, 2021

(Or merge a PR to the docs, as you choose! :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants