Skip to content

Conversation

@c-cube
Copy link
Owner

@c-cube c-cube commented Mar 27, 2024

No description provided.

@c-cube c-cube changed the title timeout input with timeout Mar 28, 2024
@c-cube c-cube marked this pull request as ready for review March 28, 2024 15:25
Bytes.create size

type decompress_state =
type transduce_state =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unusual name, I would have gone with "codec" instead of "transducer" I think. Or maybe just "compressor".

end

(** Input stream where [input] takes a timeout.
This is useful for network operations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for so many other operations!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I don't know a lot of IOs that are truly non blocking besides networking? At least on linux, which is what I know, normal IOs on the filesystem are always blocking, for example.

val map_char : (char -> char) -> #t -> t
(** Transform the stream byte by byte *)

val input_with_timeout : #t_with_timeout -> float -> bytes -> int -> int -> int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstrings here mention "read" more often than "input", maybe these functions should also be named read_*. (Not sure if that would create name clashes elsewhere.)

len := !len - n
done

method seek i = ignore (Unix.lseek fd i Unix.SEEK_SET : int)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangerous to ignore the return value. To be on the safe side, I would check that the return value is equal to i.

end = struct
open Iostream

let now_ = Unix.gettimeofday

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I remember that gettimeofday is not guaranteeed to be monotonic? That could lead to some suprises. Not sure we can get to clock_gettime via the standard library though.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly we cannot…

see: ocaml/ocaml#12858 :(

open Iostream

let now_ = Unix.gettimeofday
let short_timeout_ : float = 2.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's so relative!

| Unix.Unix_error ((Unix.EAGAIN | Unix.EWOULDBLOCK), _, _) ->
(* sleep *)
true
| Unix.Unix_error ((Unix.ECONNRESET | Unix.ESHUTDOWN | Unix.EPIPE), _, _)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure these are all possible errors, maybe add a default exception handler as well?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other exceptions I'd let bubble down, these are specifically handled here because they're the ones I can think of that related to either blocking, or the FD being actually closed.

in

let input_with_timeout t buf i len : int =
let deadline = now_ () +. t in

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the clock is non-monotonic, then the deadline can end up being in the past, but read would still be called at least once. I'm not sure how best to handle that.

Copy link

@wintersteiger wintersteiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a number of small nitpicks to complain about :-)

c-cube and others added 2 commits March 28, 2024 16:53
Co-authored-by: Christoph M. Wintersteiger <[email protected]>
Co-authored-by: Christoph M. Wintersteiger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants