janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.38k stars 217 forks source link

Doc tweaks for ev/deadline and ev/with-deadline #1410

Closed sogaiu closed 3 months ago

sogaiu commented 4 months ago

This is an attempt to address #1349.

Background can be seen in that issue, and a summary is available at this comment.

Thanks to @amano-kenji and @llmII for discussions and investigations.

amano-kenji commented 4 months ago

It is okay, but it doesn't explain the relationship between tocheck and tocancel. It doesn't document the art of choosing the right fiber for tocancel.

If I didn't read the background discussion, I would say what is this?

pepe commented 4 months ago

@amano-kenji, I would like to ask you to edit your last comment to remove vulgarism, please.

llmII commented 4 months ago

The only relationship between tocheck and tocancel that is definitive is within ev/with-deadline. With ev/deadline it is an optional but likely a typical association that the task tocancel started the fiber tocheck and is interested in receiving an error rather than waiting on tocheck to complete, and possible could respond by canceling tocheck. I'm not sure how or if the typical use case should be in the doc-string because it is possible to have something less typical. It might be that site documentation could benefit from outlining the typical usage, but I believe that if one is using the typical usage, they could likely just use ev/with-deadline. Which would of sorts, roundabouts, lead towards that one probably is using ev/deadline specifically for a special usage and thus documenting the typical usage as with ev/with-deadline would fail to make sense to do in the docstring?

sogaiu commented 4 months ago

it doesn't explain the relationship between tocheck and tocancel. It doesn't document the art of choosing the right fiber for tocancel.

AFAIU, there isn't necessarily a relationship between tochecck and tocancel.

IIUC, establishing one is entirely up to the programmer. There doesn't need to be one for this function operate.

I've started to take some notes for potential additions / changes to the website docs here.

The kind of thing you are describing might be a good addition there.

amano-kenji commented 4 months ago

If I was the author of this pull request, I would add more background information to the docstring for ev/deadline.

I would at least document how cancelling tocancel usually leads to cancellation of tocheck as well, but tocheck doesn't have to be cancelled along with tocancel in some cases. If I don't understand the art of choosing fibers for ev/deadline, I wouldn't feel comfortable about using it. People who aren't as OCD as I am would probably just avoid using ev/deadline because they don't know what tocancel is supposed to be.

They will likely think

I just passed nil to tocancel, and it just works. I don't know how it works or why it works, but it works.

What the hell is root fiber? How is it related to current fiber in any way? It doesn't make sense at all.

If cancelling tocancel doesn't cancel tocheck, is ev/deadline going to freeze for ever?

That's not good enough. This will lead to confusion or reverse-engineering. Perhaps, people can actually freeze ev/deadline for ever with a wrong fiber for tocancel.

If the background information is too verbose, then ev/deadline can refer people to an offline document.

llmII commented 4 months ago

@amano-kenji wrote:

I would at least document how cancelling tocancel usually leads to cancellation of tocheck as well, but tocheck doesn't have to be cancelled along with tocancel in some cases. If I don't understand the art of choosing fibers for ev/deadline, I wouldn't feel comfortable about using it. People who aren't as OCD as I am would probably just avoid using ev/deadline because they don't know what tocancel is supposed to be.

The problem would be that that isn't necessarily true. One can contrive ways in which this would not be the case. Say you have a task (A) and a task (B). B knows about A and knows A has an interest in the cancellations of fibers it runs under a deadline. ev/deadline allows for more uses than the typical, though ev/with-deadline outlines the typical usage and though I find it hard to have a valid use otherwise, I'm sure it will be imagined, and documentation stating the typical should probably say something more along the lines of "Typically used via ev/with-deadline, and not called manually".

What the hell is root fiber? How is it related to current fiber in any way? It doesn't make sense at all.

That's not good enough. This will lead to confusion or reverse-engineering. Perhaps, people can actually freeze ev/deadline for ever with a wrong fiber for tocancel.

If the background information is too verbose, then ev/deadline can refer people to an offline document.

Documenting what is a Fiber, and a Task, and a Root Fiber in every single function using them is not a good idea. That's way too verbose. It shouldn't be surprising or unexpected for one to glean those terms from referencing the language documents.

Edit: I did notice that a Fiber calling into the event loop after it's root fiber got canceled due to a deadline did receive the deadline error.

amano-kenji commented 4 months ago

I would likely mention something like

Cancelling tocancel must lead to cancellation of tocheck. Otherwise, ev/deadline may freeze for ever. The current fiber is cancelled when the root fiber is cancelled.

This is not too verbose. You can probably condense it down to

Cancelling tocancel must lead to cancellation of tocheck. Otherwise, ev/deadline may freeze for ever.

After reading this, people will realize cancelling the root fiber leads to cancellation of the current fiber.

llmII commented 4 months ago

I would likely mention something like

Cancelling tocancel must lead to cancellation of tocheck. Otherwise, ev/deadline may freeze for ever. The current fiber is cancelled when the root fiber is cancelled.

This is not too verbose. You can probably condense it down to

Cancelling tocancel must lead to cancellation of tocheck. Otherwise, ev/deadline may freeze for ever.

After reading this, people will realize cancelling the root fiber leads to cancellation of the current fiber.

If anything other than the root fiber of tocheck is given as tocancel then tocheck might not be canceled. Cancelling the root fiber of tocheck is no guarantee that tocheck will be canceled. If tocheck protects all it's calls into the event loop it will remain un-errored (the fiber created by protect receives the error thrown by calling into the event loop via ev/* functions).

amano-kenji commented 4 months ago

You can freeze ev/deadline unknowingly?

llmII commented 4 months ago

It should be possible. The likelihood that someone will without purposefully mucking around in ev/deadline is minimal. It's also why one should limit their error handlers to the minimum body of code needing to run within an error handler (so that the error propagates upwards). I think some of the code I displayed in my review actually illustrated that the body of ev/with-deadline can continue to run even after the root fiber of that body which is put into a fiber is cancelled. tocheck is just never directly cancelled by ev/deadline.

Basically, it counts on the fact that tocheck will either yield, or yield to the event loop at some point, entering a state from one of the following [:dead :suspended :pending].

Any fiber that doesn't do one of those things - yield, or yield to the event loop - is in danger of freezing Janet in general, not just ev/deadline, and I'm sure this is documented on Janet's site somewhere (and is general knowledge for concurrency within a single thread).

amano-kenji commented 4 months ago

I read the definition of ev/with-deadline.

(defmacro ev/with-deadline
    `Run a body of code with a deadline, such that if the code does not complete before
    the deadline is up, it will be canceled.`
    [deadline & body]
    (with-syms [f]
      ~(let [,f (coro ,;body)]
         (,ev/deadline ,deadline nil ,f)
         (,resume ,f))))

I wrote test.janet

(def fiber (coro
             (try
               (forever (ev/sleep 1))
               ([err f]
                (eprint "fiber cancelled")
                (propagate err f)))))

(def task (ev/spawn
            (try
              (forever
                (ev/sleep 1))
              # ignore errors
              ([err f]))))

(pp (ev/deadline 5 task fiber))
(print "ev/deadline returned...")
(resume fiber)

Running this script results in

$ janet test.janet
<fiber 0x561235B2B600>
ev/deadline returned...

Thus, ev/deadline returns tocancel immediately. It doesn't block. ev/with-deadline cancels coro by sending an error to the root fiber which is running tocheck right now. Because the root fiber is resuming tocheck, error is sent to tocheck through the root fiber. tocheck is not directly cancelled.

Actually, tocheck continues sleeping in the main task in test.janet.

llmII commented 4 months ago

Borrowing from my review:

This also shows the body is still alive...

(pp
  (protect
    (ev/with-deadline 0.01
      (protect (ev/sleep 2)) # the call to `ev/sleep` receives the error...
      (pp (fiber/status (fiber/current)))
      (pp [:fib (fiber/current)])
      (pp [:lineage (debug/lineage (fiber/root))])
      (flush)
      (yield :hi))))

With results:

:alive
(:fib <fiber 0x06EB603B8050>)
(:lineage @[<fiber 0x06EB6026CED0> <fiber 0x06EB603B7BF0> <fiber 0x06EB603B7FE0> <fiber 0x06EB603B8050>])
(true :hi)
nil

And

(let [fib (coro
            (pp [:fib (fiber/current)])
            (pp [:lineage (debug/lineage (fiber/root))])
            (ev/sleep 0.2)
            (yield :hi))]
  (pp [:tocancel (ev/deadline 0.1 nil fib)])
  (pp (protect (ev/sleep 5)))
  (print "Cancellation has occurred.")
  (resume fib))

Has results:

:tocancel <fiber 0x06EB6026CED0>)
(false "deadline expired")
Cancellation has occurred.
(:fib <fiber 0x06EB6026DD40>)
(:lineage @[<fiber 0x06EB6026CED0> <fiber 0x06EB6026DA30> <fiber 0x06EB6026DD40>])
:hi
llmII commented 4 months ago

@amano-kenji wrote:

I think ev/deadline should mention that it doesn't cancel tocheck and returns tocancel immediately without waiting for anything.

It already mentions in both the new documentation and the old that it is tocancel that gets canceled. It takes a lot of assumption to arrive at the conclusion that tocheck is actually canceled by ev/deadline.

The current documentation for ev/with-deadline is wrong though, and the tweaked documentation doesn't highlight this fact either. I only realized while reviewing that I was automatically interpreting the word "it" as "root fiber" and that the root fiber was (fiber/root).

amano-kenji commented 4 months ago

I think ev/deadline should mention that it returns tocancel immediately. deadline makes me assume that the function waits until the deadline before returning tocancel.

amano-kenji commented 4 months ago

The current docstring for ev/deadline says

Set a deadline for a fiber tocheck.

deadline is a terrible choice of word for tocheck. This word makes me think tocheck is cancelled beyond the deadline.

Deadline for tocheck means tocheck is dead...!! If you don't finish within deadline, you are dead. You understand me?

tocheck dead is not what happens.

llmII commented 4 months ago

That's going to argue what words mean or imply. I'm not sure if there is a better selection of a name.

In life, one often continues and completes work after a deadline has failed to be met, but it means they did the work for nothing (it's unaccepted as past due). This is exactly what happens to tocheck. It continues it's work unless told otherwise even after it failed to meet the deadline.

amano-kenji commented 4 months ago

If you don't finish fiber A within a deadline, I will cancel your mom's lunch, but I won't do anything to fiber A.

That would be easier to understand.

(ev/deadline 5-seconds your-mom's-lunch fiber-A)
amano-kenji commented 4 months ago

A better docstring would be

Wait fiber tocheck for sec seconds. If it doesn't finish in sec seconds, tocancel will be cancelled as with ev/cancel, but nothing happens to tocheck.

tocancel is the hostage. tocheck is the fiber with a deadline.

llmII commented 4 months ago

@amano-kenji

The proposed docstring (from C side) is as below:

              "Set a deadline for a fiber `tocheck`. If `tocheck` is not finished after `sec` seconds, "
              "`tocancel` will be canceled as with `ev/cancel`. Note that if `tocancel` is given, "
              "it must be a task (aka root fiber). `sec` is a number that can have a fractional part. "
              "If `tocancel` and `tocheck` are not given, they default to `(fiber/root)` and "
              "`(fiber/current)` respectively. Returns `tocancel`.") {

Your proposal for a better one fails to outline what happens to tocancel and what tocancel must be. The current proposal doesn't lack in that information. Adding an explicit note that about tocheck is unnecessary as the only thing ev/deadline does is check it's liveliness. Nowhere in the docstring is there a hint that any action is taken against tocheck. Your proposal also fudges the truth of the matter a tibt with "but nothing happens to tocheck" since that can be untrue depending on if tocancel is the ancestor of tocheck and tocheck makes event loop calls. Nothing is done to tocheck directly, true. Claiming nothing happens to tocheck due to the cancellation of tocancel can be untrue in some cases though.

amano-kenji commented 4 months ago

Set a deadline for a fiber tocheck

This wording is still confusing. This made me think tocheck dies....

I'd rather write

Set a deadline for tocancel. If tocheck doesn't finish in sec seconds, tocancel is cancelled as with ev/cancel.

I may even add

Cancelling tocancel can lead to cancellation of tocheck if tocancel is the root fiber for tocheck.

Deadline for something means death for the thing in plain english. The documentation should make it clear that death directly comes to tocancel only. If documentation isn't as obvious as llama, it becomes an IQ test.

llmII commented 4 months ago

Set a deadline for a fiber tocheck

This wording is still confusing. This made me think tocheck dies....

I'd rather write

Set a deadline for tocancel. If tocheck doesn't finish in sec seconds, tocancel is cancelled as with ev/cancel.

That would be wrong. The thing checked for liveliness beyond a deadline expiration is the thing which has a deadline set upon it. tocancel does not get checked for liveliness.

I may even add

Cancelling tocancel can lead to cancellation of tocheck if tocancel is the root fiber for tocheck.

Deadline for something means death for the thing in plain english. The documentation should make it clear that death directly comes to tocancel only.

Adding an ambiguous truth is likely less useful than adding nothing and letting it remain true that "there is nothing documented about anything being done to tocheck so we cannot assume anything is done against it".

amano-kenji commented 4 months ago

Then, this is better.

Wait fiber tocheck for sec seconds. If it doesn't finish in sec seconds, tocancel will be cancelled as with ev/cancel, but ev/deadline doesn't do anything directly to tocheck.

llmII commented 4 months ago

Then, this is better.

Wait fiber tocheck for sec seconds. If it doesn't finish in sec seconds, tocancel will be cancelled as with ev/cancel, but ev/deadline doesn't do anything directly to tocheck.

Wait is untrue, there is no waiting, according to what you said earlier, ev/deadline returns immediately.

amano-kenji commented 4 months ago

How about this?

If tocheck doesn't finish in sec seconds, tocancel will be cancelled as with ev/cancel. ev/deadline doesn't do anything directly to tocheck.

Deadline for tocheck was the thing that confused me. It can mean two things.

How do I know whether the docstring is or is not incomplete or esoteric?

I want to eliminate the possibility of connotations. Expecting users to take one interpretation over another is bad.

llmII commented 4 months ago

How about this?

If tocheck doesn't finish in sec seconds, tocancel will be cancelled as with ev/cancel. ev/deadline doesn't do anything directly to tocheck.

Deadline for tocheck was the thing that confused me. It can mean two things.

* If tocheck doesn't finish in deadline, tocancel dies.

* If tocheck doesn't finish in deadline, tocheck and tocancel die, but the docstring assumes users will arrive at a conclusion that tocheck dies, too. That assumption is a secret knowledge among inner circle. Until you are in the inner circle, you have to suffer. Only people in the inner circle have the full knowledge. Janet is an esoteric langauge. The real knowledge is shared verbally outside documentation.

I want to eliminate the possibility of connotations. Expecting users to take one interpretation over another is bad.

You're confusion probably stems from a misunderstanding of the term deadline. A deadline means the work done is no longer timely, not that it isn't done, or isn't continuing to be worked upon.

Your proposed doc string leaves out key details such as the default values, the necessary type for tocancel and probably other things (those are just the two I recognize as lacking offhand).

Finally, the second thing in your bullet list goes with the idea that people read documentation and make assumptions where the documentation outlines nothing occurring. If a behavior is not documented, it either doesn't happen, or should be documented. In this case, the documentation would align with "it doesn't happen, nothing to document".

amano-kenji commented 4 months ago

a misunderstanding of the term deadline

It is not a misunderstanding. Deadline by itself doesn't explicitly mention which things die after deadline.

In matters of death, we should explicitly mention which things are killed directly.

Oops, I forgot to mention tocheck dies, too. <--- That's what I was thinking...

llmII commented 4 months ago

A deadline implies the death of validity of work, not the death of that doing the work.

amano-kenji commented 4 months ago

A deadline has many meanings. It's not just one thing. It can mean a different thing to a different person.

One of definitions on dictionary.com says

a boundary around a military prison beyond which a prisoner could not venture without risk of being shot by the guards.

This is the definition I thought of. Deadline for prisoner A means death for prisoner A.

Unless you want to define the meaning of deadline in ev/deadline, you need to let people know that tocheck is not cancelled directly.

llmII commented 4 months ago

A deadline has many meanings. It's not just one thing. It can mean a different things to a different person.

One of definitions on dictionary.com says

a boundary around a military prison beyond which a prisoner could not venture without risk of being shot by the guards.

This is the definition I thought of. Deadline for prisoner A means death for prisoner A.

The validity of the work of life expired beyond the boundary, yes. The work is still being done minutes beyond the deadline in that case (until true death occurs, depending upon where one is shot, and it was a risk, not a guarantee).

amano-kenji commented 4 months ago

So, you just want to accept the possibilty that other people will misinterpret the docstring? That's esoteric.

I've complained about this clearly over many comments. Yet, you still dismiss my misinterpretation because when a person misinterprets, the person is wrong, but documentation is not wrong.

As you can see, my english is perfectly good enough for communication. Yet, I misunderstood. I'm not a chinese person who just started learning english 2 weeks ago. I have used english for more than 20 years. If I"m confused, many other people can be confused as well.

Documentation should not leave something to possible interpretations unless you are going to verbally communicate your understanding to every user one by one. The purpose of documentation is to eliminate the need to be there for users in person.

Deadline for tocheck alone is not acceptable unless it's accompanied by a sentence like

ev/deadline doesn't do anything directly to tocheck.

English can be ambiguous. We need to add more clarification to eliminate ambiguity. The docstring is not an IQ test. It should make things abundantly clear.

llmII commented 4 months ago

I'm not against adding that "nothing is explicitly done to tocheck beyond checking it's liveliness" (and wording may differ from mine present).

I am wary of removing the terminology "deadline" from the docstring due to possible misinterpretation. Failure to interpret the meaning of the word, even from amongst the context of the other documentation, is very unlikely, unless one makes assumptions. Assuming a deadline implies death has no bearings in reality outside of where a very specific and non-general definition is used (which means someone's picking the definition they like, vs something more general). In almost all cases I've seen deadline used as a term it implied the death of validity of work (and this is me summing things up). The first, and nearly most applicable definition on dictionary.com is as follows:

noun

    the time by which something must be finished or submitted; 
    the latest time for finishing something: a five o'clock deadline.

It nowhere implies relevance towards anything but the validity of the work, nor does it say that one doesn't continue to work on it past deadline.

amano-kenji commented 4 months ago

Assuming a deadline implies death has no bearings in reality

The docstring for ev/deadline mentions it already tries to kill tocancel with ev/cancel. Perhaps, bakpakin forgot to mention that it also tries to kill tocheck with ev/cancel. There is no way to know whether someone forgot to mention something until I ask the author directly.

llmII commented 4 months ago

I said:

I'm not against adding that "nothing is explicitly done to tocheck beyond checking it's liveliness" (and wording may differ from mine present).

@amano-kenji said:

Assuming a deadline implies death has no bearings in reality

The docstring for ev/deadline mentions it already tries to kill tocancel with ev/cancel. Perhaps, bakpakin forgot to mention that it also tries to kill tocheck with ev/cancel. There is no way to know whether someone forgot to mention something.

Which would solve your "what happens to tocheck conundrum". The key is to not say "nothing happens to tocheck" since that could be untrue, and to note that ev/deadline does not do anything to tocheck (aside from check it's liveliness) if it must be noted.

amano-kenji commented 4 months ago

nothing is explicitly done to tocheck beyond checking it's liveliness

Yes, I would like that. I'd write

Nothing is explicitly done by ev/deadline to tocheck beyond checking its liveliness.

With this, I see no other possible interpretation.

sogaiu commented 4 months ago

Still processing the content above (^^;

For the moment, my leaning is that perhaps both docstrings might be further improved.

I think ev/with-deadline's docstring is not quite correct (as pointed out by llmII).

I can see some benefit in trying to further clarify ev/deadline's docstring.

sogaiu commented 4 months ago

Based on discussions with @amano-kenji and @llmII, I've put together the following candidate for ev/deadline:


(ev/deadline sec &opt tocancel tocheck)

Schedules the event loop to try to cancel the tocancel task as with ev/cancel.

After sec seconds, the event loop will attempt cancellation of tocancel if the tocheck fiber is resumable.

sec is a number that can have a fractional part.

tocancel defaults to (fiber/root), but if specified, must be a task (root fiber).

tocheck defaults to (fiber/current), but if specified, should be a fiber.

Returns tocancel immediately.


Note: updated multiple times.

amano-kenji commented 4 months ago

That is not confusing to me.

amano-kenji commented 4 months ago

Mention that ev/with-deadline may try and fail to cancel the body of code inside it if the body discards the error.

llmII commented 4 months ago

Mention that ev/with-deadline may try and fail to cancel the body of code inside it if the body discards the error.

That's incorrect, the body of code within ev/with-deadline is the tocheck fiber in ev/deadline. No cancellation is attempted on tocheck whatsoever (otherwise, the code doing cancellation would have to figure out if it needs to call ev/cancel since it's a root fiber, or cancel since it's just a fiber).

EDIT:

For reference, code I used to exemplify this earlier:

(pp
  (protect
    (ev/with-deadline 0.01
      (protect (ev/sleep 2)) # the call to `ev/sleep` receives the error...
      (pp (fiber/status (fiber/current)))
      (pp [:fib (fiber/current)])
      (pp [:lineage (debug/lineage (fiber/root))])
      (flush)
      (yield :hi))))

Macro expanding just the ev/with-deadline part and replacing function values with their names:

(let [_00004n
      (coro
        (protect (ev/sleep 2))
        (pp (fiber/status (fiber/current)))
        (pp [:fib (fiber/current)])
        (pp [:lineage (debug/lineage (fiber/root))])
        (flush) (yield :hi))]
  # note that the coro, which is the body of code from the above
  # `ev/with-deadline` code is in the `tocheck` argument position
  # for `ev/deadline`, and `ev/deadline` does not touch `tocheck`
  # except for a liveliness check.
  (ev/deadline 0.01 nil _00004n)
  (resume _00004n))
llmII commented 4 months ago

tocheck defaults to (fiber/current), but if specified, should be a fiber.

I'll note that his is true, as a root fiber is a fiber. Any kind of fiber is fine for tocheck I believe. One could theoretically do something like the following and it'll work.

(ev/spawn
  (ev/deadline 5)
  (pp [:fiber-current (fiber/current) :fiber-root (fiber/root)])
  (print "here")
  (ev/sleep 10))

In this case, the current fiber, and the root fiber are one in the same, and is the value for tocheck and tocancel.

sogaiu commented 4 months ago

I'll note that his is true, as a root fiber is a fiber. Any kind of fiber is fine for tocheck I believe.

Yes, the intent of the text was may be not so apparent (^^;

Perhaps:

tocheck defaults to (fiber/current), but if specified, can be any type of fiber.

is an improvement?

Another possibility might be:

tocheck defaults to (fiber/current), but if specified, should be a fiber. Note that tocheck does not need to be a task or root fiber.

Where are my better terms when I need them?

llmII commented 4 months ago

@sogaiu

I prefer the first. The other is more explicit but probably more verbose than necessary I'd think.

amano-kenji commented 4 months ago

Because ev/with-deadline resumes the body inside the root fiber, the error is passed down to the body through the root fiber. So, ev/with-deadline is trying to kill the body.

To say otherwise is like saying hitler didn't try to kill people because he merely ordered nazi soldiers to kill people and he didn't kill anyone directly. Order givers share responsibility with order followers. Order givers say they didn't kill anyone. Order followers shirk responsibility because they were just following orders... No one wants to be held responsible for their actions.

ev/with-deadline is responsible for cancelling the body. I know ev/with-deadline is using the event loop as its henchman, but it is still responsible. Order givers and order followers are responsible for their actions.

llmII commented 4 months ago

If you say that, then you have to say ev/deadline is trying to kill tocheck. You're ignoring the macro's expansion and the semantics behind what is true for ev/deadline remains true for ev/with-deadline. No action is taken against the body except to check it's liveliness.

If you look at the example code, the fiber created by the body is clearly alive. Who knows if all of it's calls into the event loop are protected either because it directly does so, or finds it necessary to do so because it calls into code that can error for various reasons, or calls functions that interact with the event loop but protect it from the error generated thereby?

Nothing is done to the body because the body is tocheck in ev/deadline which ev/with-deadline expands to underneath the hood, also clearly demonstrated by the code above.

amano-kenji commented 4 months ago

ev/with-deadline still gave an order to kill the body through the event loop and the root fiber.

That's what actually happens...

ev/with-deadline put the body on the chopping block by resuming it in the root fiber.

Users actually try to cancel the body throuth ev/with-deadline.

llmII commented 4 months ago

Again, untrue, it orders the value of (fiber/root) to be canceled. That's all. Anything happening due to that is coincidence at best and not guaranteed whatsoever. Saying a function (or in this case macro) does something it clearly doesn't is to lie to the consumers of the function (or in this case, macro).

amano-kenji commented 4 months ago

But, the users actually intend to cancel the body beyond deadline..... That's what matters....

The root fiber delivers the error to the body because the body is currently resuming in the root fiber.

If ev/with-deadline didn't try to kill the body, it wouldn't resume the body in the first place.

The root fiber is the chopping block. The body is the casualty.

Hey, root fiber, hold the body for me. I'm going to order my henchman to chop it.

If that's not what happens, I don't know what happens. To me, that's exactly what happens.

llmII commented 4 months ago

The root fiber doesn't deliver the error. Only if the body calls into the event loop will it get the error. That's only if it's calls into the event loop are unprotected. There is no guarantee that it makes a call into the event loop after the deadline either.

A user may intend that ev/with-deadline cancel the body, but it doesn't ever. Otherwise it'd be calling cancel instead of ev/cancel. Implying such would leave the user in a state of shock and surprise.

amano-kenji commented 4 months ago

You have any better docstring for ev/with-deadline?

If the body doesn't yield to the event loop, ev/with-deadline would lead to a disaster.... and shouldn't be used..

The intention behind ev/with-deadline is to cancel the body, not the root fiber.

If it ends up directly cancelling the body, something is wrong.

llmII commented 4 months ago

The docstring in my review outlines exactly what the function does. Nothing else. It does not lie to the user about what the function does.

``Run a body of code with a deadline of `sec` seconds, such that if
the code does not complete before the deadline is up, the value of
`(fiber/root)` will be canceled. `sec` is a number that can have a 
fractional part.``

Perhaps it can be improved, but it should not invent fictions about what it actually does.