-
Notifications
You must be signed in to change notification settings - Fork 182
[RDY] Quick n dirty attempt at async callv #344
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
acb8268
initial implementation of Client.callv for async
rgrinberg 15ffd22
drain body in async callv
rgrinberg fa65aaf
test async callv
rgrinberg 96722d7
Change pipe_of_body return
rgrinberg dd7f457
adapt old code to use new pipe_of_body
rgrinberg 11a2d63
indentation fix
rgrinberg a19f590
Async's callv now handles body more correctly
rgrinberg 1fc7ad2
update CHANGES
rgrinberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Async convention here to raise
Failureor is there some other more descriptive exception? I'm not sure what more idiomatic Async code does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that defining an
Invalid_responseexception would be fine.But this PR doesn't introduce any kind of new error handling. This function was simply extracted out of request to be reusable in
callv.However, you remind of a good point. I think that both async's and lwt's callv shouldn't raise at all because it might be possible that raising on a bad response could possibly make it impossible for the user to retrieve the previous good responses (although the connection should be terminated regardless)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgrinberg Yes that's fine. Would your thought be that another PR would introduce a new error handling mechanism? As you point out the current error handling strategy is not very useful in the wild. It seems that one wants a
(Response.t * Body.t) Or_error.t Pipe.Reader.t Deferred.tinstead which would allow one to figure out what is going on with each response.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @trevorsummerssmith I think that's a better return type. However the connection will probably be terminated after the first error. If the body or response is corrupted or unreadable somehow it probably doesn't make much sense to attempt reading the next response. So you will get at most one error.
Just making sure that's clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is clear, and seems to me to be the correct behavior. That allows one to process, and make decisions about retries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't think that my previous characterization of the issue is correct. The current implementation makes it impossible to read all responses that were successfully written to the pipe.
Some care will be necessary however because an exception might be thrown before the corresponding response is actually processed in the pipe. But you should be able to catch it and iterate over the pipe regardless.