updated to recent tokio #37
Conversation
src/async/search.rs
Outdated
| str::from_utf8(&buf[..n]) | ||
| .map_err(|err| SearchError::from(err)) | ||
| .and_then(|text| parsing::parse_search_result(text)) | ||
| .and_then(|text| { | ||
| println!("Recv: {:?}", text); |
There was a problem hiding this comment.
yeah, this library isn't working at all for me atm, so i'm trying to work out why :-/
should be removed, but also, if you have a working environment it'd be great to test.
There was a problem hiding this comment.
I've tested your branch on my local network and everything seemed to work fine. At what point is an issue occurring? Can it find the gateway? Is it an error while trying to add a port mapping?
The async example is kinda tricky because it uses a hard-coded IP address for the local machine which isn't going to work in 99% of the cases. I was planning on rewriting the example so it asks the IP address in the command args.
There was a problem hiding this comment.
I've changed the async example to improve it, you could try merging master in your branch and test it out.
There was a problem hiding this comment.
neat, i'll have a look tonight.
it seems my hue gateway responds first to the SDP request, with a payload that is considered invalid by the parser. have been wondering whether it'd be useful to move to ssdp for more general message parsing / encoding.
aside, it's probably also worth renaming the async bits now async is a keyword.
There was a problem hiding this comment.
ahh, that's useful. i re-wrote the async search module to handle multiple responses properly and simplifed the logic / API a bit, though still can't seem to convince my network to play ball to test it :-(
There was a problem hiding this comment.
Does the synchronous version of the library work well?
There was a problem hiding this comment.
It might also be due to timeouts, I think the default timeout is 3 seconds, so if your router is slower than that it will timeout before its response.
There was a problem hiding this comment.
ahh it was a gateway misconfiguration, async stuff is working now ^_^
do you have any thoughts on the refactor? only thing that's not implemented is retries, but IMO that could be a separate PR.
There was a problem hiding this comment.
It's ok if retries aren't implemented just yet. I'll do a new review of the changes tomorrow (because it's 11 PM here) and test it out on my machine. I like what you did with SearchOptions and I'd probably add it to the synchronous version (if it's not done already).
You mentioned renaming the async module to something else, I think it makes sense because it will be a reserved keyword really soon. Not sure how to name it exactly, maybe tokio or aio.
hopefully it's clearer now too, still to integrate retries somehow
|
Oops sorry. I've been busy and I forgot about this. |
|
Thanks a lot. I'm going to make the changes we discussed tomorrow and then release |
|
It's out. |
|
neat, thanks! |
|
i like how you've lifted as an aside, the |
|
Do you mean the The |
|
fair enough, and yeah, you can swap |
the PR updates tokio to
0.1.18and hyper to0.12.28, removing explicitHandles and altering calls as required, and refactors the Search module to handle multiple responses.