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

Tokio-core migration #4

Closed
wants to merge 7 commits into from
Closed

Conversation

ashthespy
Copy link
Member

Migrated the crate over from tokio-core to tokio 0.1 and then to 0.2. These are both breaking changes. I am not sure if we should support tokio 0.1 and 0.2 with corresponding libmdns 0.3 and 0.4 versions, or go directly to tokio 0.2.

In the bigger (non librespot) view, we might want to consider making the library executor agnostic, so either async-std or tokio or anything else can be used.

@ryanpresciense I'd prefer this over the pin-project approach of #3, unless you see any issues?
We can add in the v4/v6 changes in a separate PR.

FWIW, I also migrated librespot over from tokio-core to tokio 0.1, and this works well with it, but would rather bite the bullet and get it onto tokio 0.2 and friends (hyper 0.13, futures 0.3)

@ryanpresciense
Copy link

@ashthespy Fine by me, first work I have done with the new futures. Would client functionality be a reasonable thing to bring into this project as a feature / sub crate? It would be good to have a single well maintained mdns crate in the ecosystem as it's a bit scattered at the moment. Anything avahi based is a bit of a nightmare for embedded as it's a pig to build.

@ashthespy
Copy link
Member Author

librespot per se doesn't need a client, so I doubt it would be an active priority. But @willstott101 is the official maintainer, so would be good to hear his thoughts. I don't mind as I might want to use it in another embedded project soon.. ;-)

Btw, what is the case that requires the loop (1ef93ec) I didn't see a need for that loop my tests/and with librespot.

PS: To be honest, my first work with the new stuff as well, and I struggled until I found the tokio discord ;-)

@ryanpresciense
Copy link

Without the loop for recv the task may not wake on subsequent recv. If you test it with a number of clients it's easy to reproduce.

@ashthespy
Copy link
Member Author

Without the loop for recv the task may not wake on subsequent recv. If you test it with a number of clients it's easy to reproduce.

Hmm, at some point during my testing I dropped the loop while I was playing with an async version with self.socket.recv_from(&mut buf).await?. Added it back now, could you confirm if it works properly for your use case as well?

@willstott101
Copy link
Contributor

willstott101 commented Apr 11, 2020

Hi, thanks for looking into this. And thanks for the ping @ashthespy it turns out I'd never enabled notifications for this repo... so I'd completely missed the activity.

My original agenda was to try and get the DNS packet stuff from an external crate (so basically upstream changes to dns_parser) before doing anything else. But dns_parser has diverged a lot since plietar originally did the packet building, and now looks kind of abandoned.

So I've just formatted the whole repo (including dns_parser), and made the rust 2018 edition bump in master. Hopefully that'll help reduce the noise in these diffs once re-based. I'll have a go at the re-base in a bit, since the churn is my fault :)

An mDNS client is definitely welcome in this crate @ryanpresciense . I'm not sure about a feature as it's likely a project would only need a client OR a responder. Meaning both should be features... then no-default-features would essentially be an empty crate... but perhaps that's fine.

@willstott101 willstott101 mentioned this pull request Apr 11, 2020
@willstott101
Copy link
Contributor

Ok I have pushed https://github.com/librespot-org/libmdns/tree/tokio-0.1 and https://github.com/librespot-org/libmdns/tree/tokio-0.2 to this main repo, which are your commits rebased onto master, Ash

Could you update this PR to those commits? CI passes, and it looks like your changes are pretty reasonable, but it would still be nice to see a PR without the formatting noise :)

@ashthespy
Copy link
Member Author

Will have a look tomorrow, thanks!

@ashthespy
Copy link
Member Author

@willstott101 I'm not sure what I should do now - both the branches looks like they already have the relevant commits in them?

@willstott101 willstott101 mentioned this pull request Apr 14, 2020
@willstott101
Copy link
Contributor

Yeah I guess that didn't make much sense, I've created a PR for my re-written commits myself. Let me know if it looks ok to both of you. And we can merge it in.

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.

3 participants