search

mirage / ocaml-dns

OCaml implementation of the DNS protocol
BSD 2-Clause "Simplified" License
106 stars 43 forks source link

dns-client-lwt question? #326

Closed bikallem closed 1 year ago

bikallem commented 1 year ago

Shouldn't this be the other way around? i.e. when Suspend then you sleep and when Act you call loop() again? https://github.com/mirage/ocaml-dns/blob/3e5968d38282ad949aa91527f3046bd7b84c75a8/lwt/client/dns_client_lwt.ml#LL108C27-L108C27

reynir commented 1 year ago

The first thing done in he_timer is sleeping:

    Lwt_condition.wait t.timer_condition >>= fun () ->
    loop ()
bikallem commented 1 year ago

Yes, I am aware the first thing it does is sleep. What I was inquiring about was the correct action to do after calling Happy_eyeballs.timer(). The Happy_eyeballs.timer () api documentation (https://github.com/roburio/happy-eyeballs/blob/main/src/happy_eyeballs.mli#L38) states that if the returned value is Suspend then the timer thread should sleep and that if it is Act then it should call/resume the timer thread. The lwt implementation as referenced earlier - AIUI - seems to do the opposite, i.e. suspend/sleep when Act and resume/call timer thread loop when Suspend. My question was shouldn't this be the other way around?

reynir commented 1 year ago

Ah, I understand what you mean now. I missed that there was a Lwt_unix.sleep call that you were asking about.

The documentation should be reworded. A `Suspend means that the timer task can be suspended because we don't expect any timer-related tasks - that is, until connect or connect_ip is called - hence the Lwt_condition.wait call. Of course, any remaining tasks/actions should be handled first. At the moment I don't remember if there actually will be any actions when `Suspend is returned. In the case of `Act it means we do the tasks/actions and then continue with the timer - and a timer sleeps periodically.

Perhaps we should rename `Suspend to `Timer_not_required and `Act to `Timer_still_required.

I hope this helps clarify the meaning of `Act and `Suspend. Please reach out if something is unclear. I will transfer this issue to happy-eyeballs and close it once the documentation has been improved.

reynir commented 1 year ago

Another way to put it is `Act indicates that there are still pending connections and we should keep running the timer while `Suspend indicates that we are, for now, done connecting and we can suspend or return from the timer task. In the lwt implementation we reuse the same timer task for all connections. This implementation detail has leaked into the naming of the two constructors.

hannesm commented 1 year ago

Fixed in the happy-eyeballs documentation by @reynir, thanks!