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

Don't close socket on Lwt.Canceled. #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NightBlues
Copy link

Hi! I want to write recv code with timeouts like this (just synthetic example):

 open Lwt
 
 ...
 let myrecv ~timeout conn =
   let action_th = Websocket_lwt_unix.read conn >>= fun x -> return (Some x) in
   let timeout_th = Lwt_unix.sleep timeout >>= fun () -> return None in
   Lwt.pick [ timeout_th ; action_th ]
   
 ...
 myrecv ~timeout:3.0 >>= function
 | None -> (* TODO: do something else and try again *)
 | Some x -> ...

Could you please consider this PR

@vbmithr @paurkedal

@paurkedal
Copy link
Contributor

I am not sure this would be a reliable way of implementing a timeout. If the cancel happens in between the individual read operations in websocket.read_frame, will we not end up with an out of sync stream on the input channel, from which it will be hard to recover?

@NightBlues
Copy link
Author

Not sure what does "out of sync stream" mean - you mean that we can loose some frames? For now if Lwt.cancel happens - we have invalid connection - with working fine write socket oc and closed read socket ic (and when we try to read from closed socket we get End_of_file exception)

@paurkedal
Copy link
Contributor

If you look at core/websocket.ml starting at line 278, the invocation of read_uint16 or read_uint64 is followed by two read_exactly. If the cancel happens before either of the two read_exactly, there will be bytes left on ic from that frame, which need to be discarded before processing the next frame.

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.

2 participants