Mismatch between timeout behavior and docstrings of some net/* functions?

[sogaiu]

The docstring for net/read currently has this bit:

Takes an optional timeout in seconds, after which will return nil.

In my testing, I don't observe this behavior. AFAICT, what happens seems to be that instead of returning nil after the specified timeout, an error occurs.

Here is some sample code that exhibited the behavior here (Linux box):

(def timeout 1)

(def host "127.0.0.1")
(def port "9898")

#(def s (net/listen host port))
(def s (net/server host port))

(def c (net/connect host port))

(def buf @"")
(var result :some-value)

(def start (os/time))

(try
  (set result (net/read c 1 buf timeout))
  ([e]
   (eprintf "try failed with: %n" e)))

(if (= :some-value result)
  (printf "net/read did not return")
  (printf "net/read result is %n" result))

(printf "%.02f seconds" (- (os/time) start))

(net/close s)

I get the following output with 6535d72:

try failed with: "timeout"
net/read did not return
1.00 seconds

---

It looks to me like the docstring ended up with timeout bits in this commit (2020-07-20).

I compiled janet from that commit and tried the code above. The resulting behavior was that net/read seemed to return nil:

net/read result is nil
1.00 seconds

This matches the docstring, so perhaps something has changed between then and now.

---

FWIW, in the same commit, the following net/* functions got similar changes to their docstrings regarding timeouts. Those functions were:

• net/accept
• net/read
• net/chunk
• net/write
• net/send-to
• net/recv-from

I also found that the CHANGELOG for 1.12.1 - 2020-09-07 had the following:

Change net/read, net/chunk, and net/write to raise errors in the case of failures.

This commit seems to be where the CHANGELOG had that bit added to it.

---

Any hints as to what the intended behaviors are for the six functions when a timeout is specified and the timeout is exceeded?

[bakpakin]

So the reason these are errors instead of returning nil is pretty simple - we use the janet_addtimeout function internally, which sets an internal flags JanetTimeout to; to.is_error = 1;. My guess is that with a lot of changes to the ev module early on, things like this got out of sync with the documentation.

So it's pretty easy to "fix", but it's also easy to update the documentation here, and I'm not sure which is less disruptive. I would think updating the documentation would be less disruptive and break less (no) code, and while returning nil makes certain things easier to write, an error for a timeout I think is more consistent with the kind of dynamic language that Janet is.

[bakpakin]

Yup, looking at ev/read, and ev/write, those functions have been updated with error based timeout behavior, so in this case the documentation just needs updating.

[sogaiu]

Thanks for taking a look.

I looked at a0eeb63 and it seems like five of the six docstrings are updated but not that for net/read.

May be it makes sense to make a similar change to net/read's docstring as well? I'll submit a PR along these lines.

[sogaiu]

Thank you!