Lwt: Accurate messages in exceptions raised by Lwt

[aantron]

For example, trying this in the REPL

let _, r = Lwt.wait ();;
Lwt.wakeup r ();;
Lwt.wakeup r ();;

i.e. calling Lwt.wakeup r () when r is already resolved, raises Invalid_argument "Lwt.wakeup_result". This is misleading, because Lwt.wakeup_result is a different function in Lwt. Lwt.wakeup just happens to be implemented in terms of it. This message can send a reader to the wrong part of their code. The message is generated here.

Some other instances of this are being fixed in #368, but there are many more in Lwt.

To do this, one should search Lwt for invalid_arg, fail_invalid_arg, failwith, fail_with, Failure, Invalid_argument, and make sure that the string argument to each instance of those actually reflects the API function that the user called. For helpers that get called from multiple API functions, that means the API function name will have to be passed to the helper.

cc @domsj

[raphael-proust]

I'll give this a shot this WE.

[raphael-proust]

Notes and questions:

• In unix/lwt_throttle.ml, there is call to invalid_arg inside a functor. The message raised is Lwt_throttle.S.create. I don't think there's an easy fix for that.
• In unix/lwt_log.ml, there is a call to invalid_arg inside the function render. This function is called by other functions of the module. How far do you want to take the accurate message thing? Should I add an argument to render to make the other functions accurate (and hide the argument for the exported function?
• In unix/lwt_bytes.ml, there is a use of (^) to prepare the message to invalid_arg. Most other places use Printf.ksprintf. Should I change that?
• In core/lwt_stream.ml, there is a call to invalid_arg inside a method of the class bounded_push_impl. The exception message reports it as coming from the class bounded_push which is the corresponding class type. Note sure that's a problem.
• In unix/lwt_io.ml, there are multiple cases of Invalid_argument where not only the name of the function but the value for each argument is also printed. (And it's in non-standard style as well.) I don't think argument values are useful in the error message. Should they be removed? There are a few more error messages like that: in lwt_io.ml has some Failure.
• In unix/lwt_io.ml at line 228, there is another message to improve. It's a slightly more involved one (the code paths are not immediately obvious like the other ones), I'll do it soon.

[aantron]

In unix/lwt_throttle.ml, there is call to invalid_arg inside a functor. The message raised is Lwt_throttle.S.create. I don't think there's an easy fix for that.

Agreed. That one should probably be left alone.

In unix/lwt_log.ml, there is a call to invalid_arg inside the function render. This function is called by other functions of the module. How far do you want to take the accurate message thing? Should I add an argument to render to make the other functions accurate (and hide the argument for the exported function?

I think it's fine to fix this one as you already have. It looks like render is not called by syslog, channel directly, but by callbacks that they register anyway. I agree that just using render makes the most sense here.

In unix/lwt_bytes.ml, there is a use of (^) to prepare the message to invalid_arg. Most other places use Printf.ksprintf. Should I change that?

I don't see that. Perhaps you mean another file? In any case, leaving it alone is fine, as is changing it to ksprintf.

In core/lwt_stream.ml, there is a call to invalid_arg inside a method of the class bounded_push_impl. The exception message reports it as coming from the class bounded_push which is the corresponding class type. Note sure that's a problem.

That's probably fine. We just want the user to be able to map the error message to their user code as often as possible, so this seems like the right error to report.

In unix/lwt_io.ml, there are multiple cases of Invalid_argument where not only the name of the function but the value for each argument is also printed. (And it's in non-standard style as well.) I don't think argument values are useful in the error message. Should they be removed? There are a few more error messages like that: in lwt_io.ml has some Failure.

Wow, that is weird indeed. Looks like these have been in Lwt_io since it was first added (so presumably it wasn't explicitly requested by anyone). I'd say do what you think is right. I don't think we will harm anyone by removing the arguments, and switching to a standard style.

In unix/lwt_io.ml at line 228, there is another message to improve. It's a slightly more involved one (the code paths are not immediately obvious like the other ones), I'll do it soon.

For perform_io, I think it's best to just insert perform_io, and not try to guess the user function. There are indeed too many code paths :) It's still accurate; the user will eventually realize that perform_io is an internal function, and hopefully they will also see a stack trace to show them that. One could also do Invalid_argument "perform_io (internal)", but that's not standard of course.

Thanks for working on this! I'd say you're pretty much done, at least for these files (not sure if there are more to do).

One thing I noticed is that GitHub is not associating the commit with your GH account; maybe change the email in your repo config locally?

[aantron]

(well, of course there is the stuff in lwt.ml...)

Also, we mainly want to avoid reporting

• names of functions that don't exist in Lwt
• names of other functions that exist in Lwt (like happens now with Lwt.wakeup).

It's okay to sometimes report an internal or slightly inaccurate name, as long as we are not misleading users, and users have a pretty decent chance of accurately figuring out where the string is coming from.

[raphael-proust]

Pushed a few more changes.

I scanned lwt.ml and other files; I didn't find any issues with error reporting. Specifically,

1. I grepped for invalid_arg, failwith and the likes.
2. I grepped for ".*\..*" to find strings that include a dot (as in, error messages that include a module name followed by a function name).

I might have missed some because of all the false positives.

[aantron]

Thanks for this.

The issue in lwt.ml is in the top comment. I think for that, you'd have to make the current wakeup_result and wakeup_later_result functions into helpers, and pass the name of the calling function to them.

[raphael-proust]

I tested the double-wakeup errors by temporarily modifying the tests to match not only the exception but also the string that it carries. E.g.,

  test "wait: double resolve" begin fun () ->
    let _, r = Lwt.wait () in
    Lwt.wakeup r "foo";
    try
      Lwt.wakeup r "foo";
      Lwt.return false
    with Invalid_argument "Lwt.wakeup" ->
      Lwt.return true
  end;

This causes warnings in the test suite, and so it is not committed. It could be tested without warnings. Not sure if it's necessary though.

[aantron]

+1 for choosing testing in the test suite :)

I think (and I guess you do as well :)) those warnings are not relevant to us, as we are testing Lwt's own code in its own test suite, indeed we want to know what the string arguments are.

I was actually planning to open a follow-on easy issue about testing the messages (only for lwt.ml, the other files need better test suites first). So, if you add the tests now, thank you :)

You may already know this, so I apologize, but you can suppress the warnings locally with annotations like

  test "wait: double resolve" begin fun () ->
    let _, r = Lwt.wait () in
    Lwt.wakeup r "foo";
    begin try
      Lwt.wakeup r "foo";
      Lwt.return false
    with Invalid_argument "Lwt.wakeup" ->
      Lwt.return true
    end [@ocaml.warning "-52"]    (* <------- *)
  end;

(* If you can make it less ugly, please do :) *)

We already have tests for double completion through wakeup, wakeup_exn, wakeup_later, wakeup_later_exn, but not for double completion through wakeup_result or wakeup_later_result. We should probably add two of each (since there are two result constructors).

Also, I noticed that there is an inconsistency in the test names. The wakeup tests are named like "double resolve", but the wakeup_later tests are named like "already resolved", so maybe it's good to "resolve" that, in any way you please :)

[raphael-proust]

I separated the double wakeup tests from the task/wait tests. The names are somewhat consistent.

I went for suppressing the warning on the whole, existing begin/end block. Which is marginally shorter.

Technically, we should also test that when woken up twice but with different functions, it's always the second function's name that is reported. That's 30 combinations which is a bit much I think. Actually, inconsistently, some of the tests use a different initial wakeup functions. I'll clean that up.

[aantron]

I agree that all combinations is too much. Also, it doesn't really matter what the initial wakeup function is – we just need to get each promise into a completed state at the beginning of the test. In fact, it might make the most sense to just create a trivial promise (Lwt.return or Lwt.fail), to be as clear as possible – but don't spend any more time on this aspect of the testing than you already have (unless you want to go for purity, which I don't object to :)).