Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Lwt_unix.files_of_directory dir" fails if "dir" can't be read #265

Closed
hcarty opened this issue Jul 12, 2016 · 5 comments
Closed

"Lwt_unix.files_of_directory dir" fails if "dir" can't be read #265

hcarty opened this issue Jul 12, 2016 · 5 comments

Comments

@hcarty
Copy link
Contributor

hcarty commented Jul 12, 2016

I'm not sure how to handle this kind of failure in Lwt's current state (2.5.2) since it happens in the generation of the stream, presumably on

opendir path >>= fun handle ->

One change would be to open the directory before returning the stream, changing the type to:

val files_of_directory : string -> string Lwt_stream.t Lwt.t

or add a new function with the new signature.

Example of the failure from utop:

#require "lwt.unix";;
# Lwt_unix.files_of_directory "/doesnotexist";;
- : bytes Lwt_stream.t = <abstr>
# Lwt_unix.files_of_directory "/root" |> Lwt_stream.iter_s Lwt_io.printl;;
Exception: Unix.Unix_error (Unix.ENOENT, "opendir", "/root").
@hcarty
Copy link
Contributor Author

hcarty commented Jul 12, 2016

I also tried:

Lwt_unix.files_of_directory "/doesnotexist"
|> Lwt_stream.map_exn
|> Lwt_stream.filter_map (function Lwt_stream.Value v -> Some v | Lwt_stream.Error _ -> None)
|> Lwt_stream.iter_s Lwt_io.printl

but that ends up hanging indefinitely.

@aantron
Copy link
Collaborator

aantron commented Jul 18, 2016

The proposed type signature would capture the behavior of files_of_directory more accurately. However, I am not sure about this problem – can't you simply wrap reads of the stream in Lwt.catch? Or are you passing these streams to some code that doesn't, and shouldn't, know about this failure mode?

@hcarty
Copy link
Contributor Author

hcarty commented Jul 18, 2016

The code using the stream does not and should not know about this failure state. That said, I'm not sure how to properly capture this failure in a sane way since this hangs rather than capturing the error as I would have expected it to. Do you have any suggestions for a better approach?

@aantron
Copy link
Collaborator

aantron commented Jul 18, 2016

Ok, then this needs to be addressed.

In the filter_map example, I guess you first get a stream "filled" with the same exception forever. That's because files_of_directory "/doesnotexist" keeps raising that exception each time it is read. After filter_map, you get a stream with no elements that never ends, which nonetheless requires work to compute.

I guess the specific effect you seem to be trying to achieve can be solved with an until_map function in Lwt_stream. However, I'm pretty hesitant to mess with Lwt_stream for the time being, as it needs that hard look (#250). See also #155.

In the meantime:

open Lwt.Infix

let until_map : ('a -> 'b option) -> 'a Lwt_stream.t -> 'b Lwt_stream.t =
  fun f s ->
    Lwt_stream.from (fun () ->
      Lwt_stream.get s >|= function
        | None -> None
        | Some v -> f v)

let () =
  Lwt_unix.files_of_directory "/"
  |> Lwt_stream.map_exn
  |> until_map (function
    | Lwt_stream.Value v -> Some v
    | Lwt_stream.Error _ -> None)
  |> Lwt_stream.iter_s Lwt_io.printl
  |> Lwt_main.run

I want to leave this issue open as an example of the kind of things that need to be considered when working on Lwt_stream.

Also, Lwt_stream has its own result type. We should probably eliminate that in a separate issue. There is also an internal one in Lwt_preemptive IIRC.

@aantron aantron added this to the Lwt_stream milestone Jul 18, 2016
@aantron
Copy link
Collaborator

aantron commented Dec 5, 2016

Closing this issue as there is a fairly reasonable solution. For current Lwt, in the above code, Lwt_stream.Value should be Result.Ok, and Lwt_stream.Error should be Result.Error. This issue is linked from #250, for the future Lwt_stream work.

Also, files_of_directory may itself eventually be deprecated (see #292).

@aantron aantron closed this as completed Dec 5, 2016
hcarty pushed a commit to hcarty/lwt that referenced this issue Dec 25, 2016
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

No branches or pull requests

2 participants