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

remove use of futures and tokio from within client #1

Merged
merged 1 commit into from
Apr 28, 2018

Conversation

astraw
Copy link
Contributor

@astraw astraw commented Apr 26, 2018

This removes an invalid use of futures: previously, the client would
create its own instance of a tokio_core reactor core and synchronously
block on this core while waiting for a response. With recent releases of
futures, this results in the error "cannot recursively call into
Core". This is discussed at
tokio-rs/tokio-core#319 and was apparently
never correct behavior.

To solve the problem and continue to use futures, it would be necessary
to allow the client to spawn new futures on an existing executor.
However, because the futures API is in flux right now and the current
behavior was simply to block synchronously on the response anyway, I
simply reverted the use of hyper to an older, pre-futures version and
kept the public API of xml-rpc unchanged.

In the future (e.g. once the futures ecosystem has settled down), it
would be good to remove the blocking wait, but for now this works for
me.

This removes an invalid use of futures: previously, the client would
create its own instance of a tokio_core reactor core and synchronously
block on this core while waiting for a response. With recent releases of
futures, this results in the error "cannot recursively call into
`Core`". This is discussed at
tokio-rs/tokio-core#319 and was apparently
never correct behavior.

To solve the problem and continue to use futures, it would be necessary
to allow the client to spawn new futures on an existing executor.
However, because the futures API is in flux right now and the current
behavior was simply to block synchronously on the response anyway, I
simply reverted the use of hyper to an older, pre-futures version and
kept the public API of xml-rpc unchanged.

In the future (e.g. once the futures ecosystem has settled down), it
would be good to remove the blocking wait, but for now this works for
me.
@adnanademovic
Copy link
Owner

The hyper 0.10-0.11 was a real pain because of a total architectural change, so I just followed the tutorial on the site to replace existing code.

This should work well enough. If you have a good source for using Rust's future libraries and tokio, that would be great! I'm sure that the intent is for futures to be... the future.

@adnanademovic adnanademovic merged commit 9891a28 into adnanademovic:master Apr 28, 2018
@adnanademovic
Copy link
Owner

I cannot publish this as is now, since hyper10 is not on crates.io. Maybe I should transition the whole hyper dependency back down to 10, and just make that hyper.

@astraw
Copy link
Contributor Author

astraw commented Apr 29, 2018

Ah yes, I only tested a local copy and forgot that hyper10 would have to be published with this approach. Still, I think that is perhaps the right thing to do because the rest of xml-rpc-rs is using hyper 0.11 and works fine, so I don't think there's a good reason to revert it. Furthermore, this would require changing the API of xml-rpc-rs (because hyper 0.11 types are in it). Therefore, I think we should publish a hyper10 crate. For prior art, I used futures2 here and here. Do these arguments make sense to you? If so, do you want to publish hyper10 or shall I?

BTW, publishing a "hyper10" shim would not be necessary if we could rename dependencies in the Cargo manifest.

@adnanademovic
Copy link
Owner

Yes, I've spent some time yesterday trying to see if there is any clean solution.

I'll go put hyper10 on crates.io. I plan to do it straight out of this repo, so it would be better if I have the publishing rights of all crates contained in this repo.

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