starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #524] Refactor all uses of `tokio::executor::spawn` into background tasks #218

New issue

Closed

opened 2026-03-07 22:50:53 +03:00 by kerem · 7 comments

kerem commented

2026-03-07 22:50:53 +03:00

Owner

Originally created by @bluejekyll on GitHub (Jul 9, 2018).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/524

See Background in resolver.

client_future::ClientFuture::with_timeout
name_server_pool::ConnectionProvider::new_connection()

Originally created by @bluejekyll on GitHub (Jul 9, 2018). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/524 See Background in resolver. - [x] `client_future::ClientFuture::with_timeout` - [x] `name_server_pool::ConnectionProvider::new_connection()`

kerem

2026-03-07 22:50:53 +03:00

closed this issue
added the
crate:all

cleanup
labels

kerem commented

2026-03-07 22:51:09 +03:00

Author

Owner

@hawkw commented on GitHub (Jul 9, 2018):

I've started on this in a branch but it may take some time, as it looks like the change will affect a lot of code.

@hawkw commented on GitHub (Jul 9, 2018): I've started on this [in a branch](https://github.com/bluejekyll/trust-dns/compare/master...hawkw:eliza/no-spawn) but it may take some time, as it looks like the change will affect a lot of code.

kerem commented

2026-03-07 22:51:14 +03:00

Author

Owner

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

Oh, nice! Thanks for picking this up. We could split this into two different issues, one for name_server_pool and the other for client_future if you want. The name_server_pool one would help clean up some of the Resolver Background stuff you wrote before.

When I was doing some recent refactoring, I was trying to pull a lot of that up the stack...

@bluejekyll commented on GitHub (Jul 9, 2018): Oh, nice! Thanks for picking this up. We could split this into two different issues, one for name_server_pool and the other for client_future if you want. The name_server_pool one would help clean up some of the Resolver Background stuff you wrote before. When I was doing some recent refactoring, I was trying to pull a lot of that up the stack...

kerem commented

2026-03-07 22:51:19 +03:00

Author

Owner

@hawkw commented on GitHub (Jul 9, 2018):

We could split this into two different issues, once for name_server_pool and the other for client_future if you want.

Yeah, that was the plan. I think the client_future change is not going to be terribly difficult, but it looks like a lot of other code that depends on it will need to be changed as well.

The name_server_pool one would help clean up some of the Resolver Background stuff you wrote before.

Yeah, I'm hoping that will be able to be simplified at least a bit!

@hawkw commented on GitHub (Jul 9, 2018): > We could split this into two different issues, once for name_server_pool and the other for client_future if you want. Yeah, that was the plan. I think the `client_future` change is not going to be terribly _difficult_, but it looks like a lot of other code that depends on it will need to be changed as well. > The name_server_pool one would help clean up some of the Resolver Background stuff you wrote before. Yeah, I'm hoping that will be able to be simplified at least a bit!

kerem commented

2026-03-07 22:51:25 +03:00

Author

Owner

@hawkw commented on GitHub (Jul 10, 2018):

Another update: I started this morning on changing name_server_pool::ConnectionProvider::new_connection() to return a background future, but have run in to a bit of an issue: the try_reconnect function in NameServer currently also calls ConnectionProvider::new_connection if reconnecting is necessary, and the background future returned will need to be spawned again any time the NameServer reconnects due to an error.

To solve this, I'm going to try to move the reconnect logic into the background future itself, so that it need only be spawned once and that future will handle reconnecting as necessary. However, I think that's a slightly bigger refactor than I had initially expected, so this may take a little time.

@hawkw commented on GitHub (Jul 10, 2018): Another update: I started this morning on changing `name_server_pool::ConnectionProvider::new_connection()` to return a background future, but have run in to a bit of an issue: the `try_reconnect` function in `NameServer` currently _also_ calls `ConnectionProvider::new_connection` if reconnecting is necessary, and the background future returned will need to be spawned again any time the `NameServer` reconnects due to an error. To solve this, I'm going to try to move the reconnect logic into the background future itself, so that it need only be spawned once and that future will handle reconnecting as necessary. However, I think that's a _slightly_ bigger refactor than I had initially expected, so this may take a little time.

kerem commented

2026-03-07 22:51:30 +03:00

Author

Owner

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

Yeah, that makes sense. I also think it makes sense to keep the reconnect logic inside the background task. I think the only thing to consider is to make sure reconnect only is attempted after a poll (lazily) and not (eagerly) immediately after a failure, if that makes sense.

Making the background basically a set of states, Disconnected (start) -> Connected <-> Failed -> Disconnected, or something like that seems like it would make the most sense.

@bluejekyll commented on GitHub (Jul 10, 2018): Yeah, that makes sense. I also think it makes sense to keep the reconnect logic inside the background task. I think the only thing to consider is to make sure reconnect only is attempted after a poll (lazily) and not (eagerly) immediately after a failure, if that makes sense. Making the background basically a set of states, Disconnected (start) -> Connected <-> Failed -> Disconnected, or something like that seems like it would make the most sense.

kerem commented

2026-03-07 22:51:35 +03:00

Author

Owner

@hawkw commented on GitHub (Jul 10, 2018):

Yeah, that's more or less what I had in mind.

Currently, I'm doing some thinking about how to structure this so that it doesn't introduce an additional mpsc channel between a NameServer and the connection, but I'm not sure whether or not that will be possible.

@hawkw commented on GitHub (Jul 10, 2018): Yeah, that's more or less what I had in mind. Currently, I'm doing some thinking about how to structure this so that it doesn't introduce an additional `mpsc` channel between a `NameServer` and the connection, but I'm not sure whether or not that will be possible.

kerem commented

2026-03-07 22:51:40 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jul 11, 2018):

I just cleaned up a few of those... and yes, it's hard in this context not to need that.

@bluejekyll commented on GitHub (Jul 11, 2018): I just cleaned up a few of those... and yes, it's hard in this context not to need that.

kerem referenced this issue

2026-03-16 01:59:26 +03:00

[PR #218] [MERGED] Remove private key and certificate generation features from the server. #1306