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

CA-289145: close socket if error occurs when using lwt connect #40

Merged
merged 1 commit into from
May 23, 2018

Conversation

krizex
Copy link
Member

@krizex krizex commented May 21, 2018

Signed-off-by: Yang Qian yang.qian@citrix.com

@@ -47,7 +47,9 @@ module M = struct
| Unix.Unix_error((Unix.ECONNREFUSED | Unix.ECONNABORTED | Unix.ENOENT), _, _) ->
Lwt_unix.sleep 5. >>= fun () ->
loop ()
| e -> fail e
| e ->
Lwt_unix.close fd;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the fd has been closed at line 43 this will fail with Unix.EBADF and shadow the real exception below. You should probably put it in a Lwt.catch ignoring any error in this line

Copy link
Collaborator

@mseri mseri May 21, 2018

Choose a reason for hiding this comment

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

Also don't use the semicolon but bind it to the Lwt.fail below

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

| e -> fail e
) in
| e ->
Lwt_unix.close fd >>= fail e
Copy link
Collaborator

Choose a reason for hiding this comment

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

>>= should be >>= fun () ->

Lwt_unix.close fd >>= fail e
)
>>= fun () ->
let ic = Lwt_io.of_fd ~close:(fun () -> Lwt_unix.close fd) ~mode:Lwt_io.input fd in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth mentioning that (fun () -> Lwt_unix.close fd) is the default value for ?close, but I'd keep it there for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of the code is quite clear. I think we don't need to add any more comment for it.
If David omitted the ~close parameter when constructing ic, then I think he should add note here that says "the default value of ?close is to close the fd"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine anyways

Copy link
Collaborator

@mseri mseri left a comment

Choose a reason for hiding this comment

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

As with some other PRs, please fix the typo and once the Travis build passes feel free to merge

@mseri
Copy link
Collaborator

mseri commented May 22, 2018

Travis is failing with

# File "lwt/protocol_lwt.ml", line 41, characters 6-534:
# Error: This expression has type
#          (Lwt_io.input Lwt_io.channel * Lwt_io.output Lwt_io.channel) Lwt.t
#        but an expression was expected of type unit Lwt.t
#        Type Lwt_io.input Lwt_io.channel * Lwt_io.output Lwt_io.channel
#        is not compatible with type unit 

There is a type mismatch between the return types. I think the easiest solution is to put back the return (ic, oc) inside the catch call

Signed-off-by: Yang Qian <yang.qian@citrix.com>
@krizex
Copy link
Member Author

krizex commented May 23, 2018

Updated, we just need to check the exception when connecting.

@coveralls
Copy link

coveralls commented May 23, 2018

Coverage Status

Coverage decreased (-0.03%) to 55.114% when pulling 243e5bc on krizex:CA-289145 into 4f2df23 on xapi-project:master.

Copy link
Collaborator

@mseri mseri left a comment

Choose a reason for hiding this comment

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

I don’t understand how we decreased coverage

@krizex
Copy link
Member Author

krizex commented May 23, 2018

I think that's because adding 12 lines while deleting 10 lines....

@mseri mseri merged commit 4ed7162 into xapi-project:master May 23, 2018
edwintorok pushed a commit to edwintorok/message-switch that referenced this pull request Sep 20, 2021
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
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.

None yet

3 participants