starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 03:05:51 +03:00

[GH-ISSUE #385] Port to tokio #472

New issue

Closed

opened 2026-03-15 22:42:06 +03:00 by kerem · 15 comments

kerem commented

2026-03-15 22:42:06 +03:00

Owner

Originally created by @Keruspe on GitHub (Apr 9, 2018).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/385

tokio-core is being replaced with tokio. Is there any plan to port trust-dns in the near future?

Originally created by @Keruspe on GitHub (Apr 9, 2018). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/385 tokio-core is being replaced with tokio. Is there any plan to port trust-dns in the near future?

kerem

2026-03-15 22:42:06 +03:00

closed this issue
added the
crate:all

cleanup
labels

kerem commented

2026-03-15 22:42:22 +03:00

Author

Owner

@bluejekyll commented on GitHub (Apr 9, 2018):

I haven’t started considering yet the implications of doing this to the library, yet. I’ve read about it, and it didn’t look necessary. Could you inform me as to the benefits that you see in needing this now?

We definitely need to keep the library up to date with the ecosystem, though I don’t want to get into unnecessary churn until the path is clear, as these are distractions from the actual core features I want to work on.

I will of course look at any PRs people would like to submit, and we can decide from there. Me personally, I might not have time to get to this before some other higher priority things I want to hit.

@bluejekyll commented on GitHub (Apr 9, 2018): I haven’t started considering yet the implications of doing this to the library, yet. I’ve read about it, and it didn’t look necessary. Could you inform me as to the benefits that you see in needing this now? We definitely need to keep the library up to date with the ecosystem, though I don’t want to get into unnecessary churn until the path is clear, as these are distractions from the actual core features I want to work on. I will of course look at any PRs people would like to submit, and we can decide from there. Me personally, I might not have time to get to this before some other higher priority things I want to hit.

kerem commented

2026-03-15 22:42:27 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 10, 2018):

For my specific use case the reason is quite simple:

I use ResolverFuture::from_system_conf which currently takes a tokio_core::reactor::Handle parameter.
If I want to port my code to using tokio, I cannot pass that Handle anymore. With tokio, UdpStream and TcpStream don't require a Handle anymore so that parameter wouldn't be necessary anymore.

@Keruspe commented on GitHub (Apr 10, 2018): For my specific use case the reason is quite simple: I use `ResolverFuture::from_system_conf` which currently takes a `tokio_core::reactor::Handle` parameter. If I want to port my code to using tokio, I cannot pass that Handle anymore. With tokio, UdpStream and TcpStream don't require a Handle anymore so that parameter wouldn't be necessary anymore.

kerem commented

2026-03-15 22:42:32 +03:00

Author

Owner

@bluejekyll commented on GitHub (Apr 10, 2018):

I was afraid that was the answer. I’ll be upgrading when I have a moment, but that may not be until the middle of May. It might be a large annoying change, and lots of old poorly documented code in certain spots, but if you wanted to take a stab at doing it, I’d gladly accept a PR.

@bluejekyll commented on GitHub (Apr 10, 2018): I was afraid that was the answer. I’ll be upgrading when I have a moment, but that may not be until the middle of May. It might be a large annoying change, and lots of old poorly documented code in certain spots, but if you wanted to take a stab at doing it, I’d gladly accept a PR.

kerem commented

2026-03-15 22:42:37 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 10, 2018):

There is no rush in this, tokio is pretty young still, I just wanted to make sure it's on the roadmap as there didn't seem to be any issue about it.

I might give it a try at some point to see the implications

@Keruspe commented on GitHub (Apr 10, 2018): There is no rush in this, tokio is pretty young still, I just wanted to make sure it's on the roadmap as there didn't seem to be any issue about it. I might give it a try at some point to see the implications

kerem commented

2026-03-15 22:42:42 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 12, 2018):

Note that latest versions of tokio-core actually use tokio underneath, and the porting can be done incrementally. A library or a binary can be ported to tokio once all its dependencies have been ported, but there is no problem in having part of the dependencies using tokio, the other part using tokio-core, as long as you still use tokio-core.

So the core can be ported first, then the rest, piece by piece

@Keruspe commented on GitHub (Apr 12, 2018): Note that latest versions of tokio-core actually use tokio underneath, and the porting can be done incrementally. A library or a binary can be ported to tokio once all its dependencies have been ported, but there is no problem in having part of the dependencies using tokio, the other part using tokio-core, as long as you still use tokio-core. So the core can be ported first, then the rest, piece by piece

kerem commented

2026-03-15 22:42:47 +03:00

Author

Owner

@cssivision commented on GitHub (Apr 21, 2018):

this is awesome, I can help with this upgrade.

@cssivision commented on GitHub (Apr 21, 2018): this is awesome, I can help with this upgrade.

kerem commented

2026-03-15 22:42:52 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 23, 2018):

@cssivision first step will be to port trust-dns-proto to tokio if you want to give it a try.

@Keruspe commented on GitHub (Apr 23, 2018): @cssivision first step will be to port trust-dns-proto to tokio if you want to give it a try.

kerem commented

2026-03-15 22:42:57 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 26, 2018):

The last tricky bit once #432 is merged is porting DnsFuture to using tokio_executor::spawn instead of tokio_core::reactor::Handle::spawn.
What makes it tricky is that the tokio_executor version requires the Future to be Send, which in turns requires all of its dependents to be Send too, and some to be Sync and you can easily get lost with lots of messy compiler errors when trying to do so.

@Keruspe commented on GitHub (Apr 26, 2018): The last tricky bit once #432 is merged is porting DnsFuture to using `tokio_executor::spawn` instead of `tokio_core::reactor::Handle::spawn`. What makes it tricky is that the tokio_executor version requires the Future to be Send, which in turns requires all of its dependents to be Send too, and some to be Sync and you can easily get lost with lots of messy compiler errors when trying to do so.

kerem commented

2026-03-15 22:43:03 +03:00

Author

Owner

@bluejekyll commented on GitHub (Apr 26, 2018):

One issue with Sync is the usage of error-chain which at the moment is not. There’s a PR on error-chain that hasn’t been merged yet, that might fix this: https://github.com/rust-lang-nursery/error-chain/pull/241

The other option is to switch to Failure now, another big port (the errors in trust-dns are old, and definitely deserve a cleanup)...

For Send, everything should be Send, and for a different reason, I was trying to track down why it’s not Send now, and I think it was related to Handle. It’s possible that if we go through and fix this, #430, then we might be Send (though once TCP connections are established I’m not sure if those would deny Send). I wasn’t careful about this before, and now it’s coming back to haunt.

——

With the current_thread option, we should discuss what the implications of that might be to the library.

@bluejekyll commented on GitHub (Apr 26, 2018): One issue with Sync is the usage of error-chain which at the moment is not. There’s a PR on error-chain that hasn’t been merged yet, that might fix this: https://github.com/rust-lang-nursery/error-chain/pull/241 The other option is to switch to Failure now, another big port (the errors in trust-dns are old, and definitely deserve a cleanup)... For Send, everything should be Send, and for a different reason, I was trying to track down why it’s not Send now, and I think it was related to Handle. It’s possible that if we go through and fix this, #430, then we might be Send (though once TCP connections are established I’m not sure if those would deny Send). I wasn’t careful about this before, and now it’s coming back to haunt. —— With the current_thread option, we should discuss what the implications of that might be to the library.

kerem commented

2026-03-15 22:43:08 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 26, 2018):

The current_thread option is not applicable actually, as you need to actually do some run too.

The use of Rc makes us !Send too, Arc fixes this easily.

I'm also pretty sure that everywhere we have stuff like Box<Future> we need to explicitely state Box<Future + Send>.

Note that once this is done, only tokio_core::reactor::Core will still be used from the tokio-core crate which is not a blocker. We probably won't be able to port those yet anyway as there is nothing like Core::run (with tokio::run you can only have Future<(), ()>).

@Keruspe commented on GitHub (Apr 26, 2018): The current_thread option is not applicable actually, as you need to actually do some `run` too. The use of `Rc` makes us !Send too, `Arc` fixes this easily. I'm also pretty sure that everywhere we have stuff like `Box<Future>` we need to explicitely state `Box<Future + Send>`. Note that once this is done, only tokio_core::reactor::Core will still be used from the tokio-core crate which is not a blocker. We probably won't be able to port those yet anyway as there is nothing like Core::run (with tokio::run you can only have `Future<(), ()>`).

kerem commented

2026-03-15 22:43:13 +03:00

Author

Owner

@bluejekyll commented on GitHub (Apr 26, 2018):

Yes, that's another cleanup to happen. Really, all of those Box<Future...> should be converted to actual future types... I Boxed a lot to get things done, so now it's a matter of going through and unboxing to actual types. I was thinking we might do impl Trait for this, but then we'll be pinning new versions to a very current release of rustc... that might be ok, not sure.

@bluejekyll commented on GitHub (Apr 26, 2018): Yes, that's another cleanup to happen. Really, all of those `Box<Future...>` should be converted to actual future types... I Boxed a lot to get things done, so now it's a matter of going through and unboxing to actual types. I was thinking we might do `impl Trait` for this, but then we'll be pinning new versions to a very current release of rustc... that might be ok, not sure.

kerem commented

2026-03-15 22:43:18 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 30, 2018):

I started digging into how we could fix the errors from dns_future when using tokio-executor in the resolver module.
I fixed two tests as examples on how it should now work: https://github.com/Keruspe/trust-dns/tree/tokio-executor

@bluejekyll what do you think of this?
I didn't fully look at the implications, but I think a good way to make lots of tests "magically" pass is to make ResolverFuture constructors return a lazy future

@Keruspe commented on GitHub (Apr 30, 2018): I started digging into how we could fix the errors from dns_future when using tokio-executor in the resolver module. I fixed two tests as examples on how it should now work: https://github.com/Keruspe/trust-dns/tree/tokio-executor @bluejekyll what do you think of this? I didn't fully look at the implications, but I think a good way to make lots of tests "magically" pass is to make ResolverFuture constructors return a lazy future

kerem commented

2026-03-15 22:43:23 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 30, 2018):

Just tried this, all the resolver tests pass now.
One client doc test doesn't pass, several integration-tests and some server ones, everything else pass just fine

@Keruspe commented on GitHub (Apr 30, 2018): Just tried this, all the resolver tests pass now. One client doc test doesn't pass, several integration-tests and some server ones, everything else pass just fine

kerem commented

2026-03-15 22:43:28 +03:00

Author

Owner

@Keruspe commented on GitHub (Apr 30, 2018):

Everything seems to work fine now, without the threading + run hack

@Keruspe commented on GitHub (Apr 30, 2018): Everything seems to work fine now, without the threading + run hack

kerem commented

2026-03-15 22:43:33 +03:00

Author

Owner

@Keruspe commented on GitHub (May 2, 2018):

I also have some wip for migrating away from tokio_core::reactor::Core, works fine for some modules, a couple of tests fail for other, requires a feature that'll be in next release of tokio to fix them, it seems.

@Keruspe commented on GitHub (May 2, 2018): I also have some wip for migrating away from tokio_core::reactor::Core, works fine for some modules, a couple of tests fail for other, requires a feature that'll be in next release of tokio to fix them, it seems.

kerem referenced this issue

2026-03-15 22:48:31 +03:00

[GH-ISSUE #472] resolver lookup_ip block forever when use different tokio runtime. #497

kerem referenced this issue

2026-03-16 02:08:23 +03:00

[PR #487] [MERGED] resolver: Split ResolverFuture into ResolverHandle and background task #1472