-
Notifications
You must be signed in to change notification settings - Fork 59
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
Allow req_perform_stream()
to perform non-blocking reads
#520
Conversation
The default behaviour stays the same, but you can now specify `wait_for` in order to switch to a non-blocking request and call the callback as the data arrives. Fixes #519
line = function(bytes) which(bytes == charToRaw("\n")), | ||
sse = function(bytes) { | ||
is_newline <- which(bytes == charToRaw("\n")) | ||
intersect(is_newline, is_newline + 1) |
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.
That's clever
Have you tried this with a real SSE server and do you end up with an extra callback with "\n" at the end? If "yes" and "no", LGTM! |
req_url_query(chunk_size = 1024) %>% | ||
req_perform_stream(accumulate_bytes, wait_for = 0) | ||
expect_equal(sum(bytes), 102400) | ||
# I'm not sure why this is so much smaller than 100, but I suspect it |
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.
@jcheng5 does this explanation make sense to you
The default behaviour stays the same, but you can now specify
wait_for
in order to switch to a non-blocking request and call the callback as the data arrives.Fixes #519
@jcheng5 Does that feel like the right argument name? I have kept the behaviour the same as previously, but maybe the default should be
wait_for = 0
? Does thewait_for
implementation loop look ok to you?