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

CRAN release broke curl #123

Closed
jeroen opened this issue Apr 20, 2018 · 9 comments
Closed

CRAN release broke curl #123

jeroen opened this issue Apr 20, 2018 · 9 comments

Comments

@jeroen
Copy link
Contributor

jeroen commented Apr 20, 2018

The curl package has a function curl_echo() that was broken with the new httpuv release.

library(curl)
example(curl_echo)
 Error in curl_fetch_memory(paste0("http://localhost:", port, "/"), handle = handle) : 
  Callback aborted 

I think the problem is that previously httpuv::service() always returned TRUE in case of success. However as if the new version it randomly returns FALSE and TRUE.

Is this changed expected?

@wch
Copy link
Collaborator

wch commented Apr 20, 2018

Currently, service() calls later::run_now(), which, according to the docs returns:

A logical indicating whether any callbacks were actually run.

In the past, it basically called uv_run() (from libuv). If you called service() without setting a timeout value, or with a non-NA value, it called uv_run() with UV_RUN_ONCE: https://github.com/rstudio/httpuv/blob/v1.3.6.2/src/httpuv.cpp#L405

The return values uv_run() with UV_RUN_ONCE: http://docs.libuv.org/en/v1.x/loop.html#c.uv_run

UV_RUN_ONCE: Poll for i/o once. Note that this function blocks if there are no pending callbacks. Returns zero when done (no active handles or requests left), or non-zero if more callbacks are expected (meaning you should run the event loop again sometime in the future).

So if I'm understanding it correctly, in the past, it should have returned TRUE only if there were more callbacks for libuv to process. I'll experiment with it some more though.

@wch
Copy link
Collaborator

wch commented Apr 20, 2018

FWIW, in httpuv 1.3.6.2, in a clean R session, this is what I get:

> print(httpuv::service())
[1] TRUE
> print(httpuv::service())
[1] FALSE
> print(httpuv::service())
[1] FALSE

@jeroen I think you were relying on some incidental behavior of service() that happened to work for your case. We could have it always return TRUE. Would that fix your problem, or does curl expect service() to return FALSE in some cases?

@jeroen
Copy link
Contributor Author

jeroen commented Apr 20, 2018

I think my assumption was that httpuv::service() would return TRUE on success, and FALSE on failure, for example if there is nothing to service.

@wch
Copy link
Collaborator

wch commented Apr 20, 2018

We currently use later::run_now(), which returns TRUE or FALSE depending on whether any callbacks are run -- but these callbacks could be from anywhere, not just httpuv.

If we make service() always return TRUE, will that work for you?

@jeroen
Copy link
Contributor Author

jeroen commented Apr 20, 2018

Yes I guess that would work. I don't fully recall what edge case I was trying to cover.

@wch wch closed this as completed in 289d161 Apr 20, 2018
@wch
Copy link
Collaborator

wch commented Apr 20, 2018

OK, done.

@jeroen
Copy link
Contributor Author

jeroen commented Apr 20, 2018

Thanks! So there is no easy way to make it return FALSE when there is no server running? For example when httpuv::startServer() was never called or it crashed?

@wch
Copy link
Collaborator

wch commented Apr 20, 2018

One complication is that you now can have multiple servers running at the same time. So if the check is, "is a server running?", and the answer is yes, you can't be sure that it's your server or someone else's.

startServer has always returned a handle to the server though. So can you do something along these lines?

handle <- NULL
try( handle <- startServer(...) )


if (!is.null(handle)) {
  # Do something
}

Note that if startServer() fails (for example, if the port is already occupied), it'll throw an error.

All that said, it might make sense to add a function that returns a list of running servers.

@jeroen I'll also forward you an email I wrote about the changes to httpuv.

@jeroen
Copy link
Contributor Author

jeroen commented Apr 21, 2018

OK thanks. In my code I run only one server. I think the main case I was trying to cover is checking if the server is still alive. But I suppose that never worked the way I thought it did.

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