sbstp / rust-igd

Internet Gateway Device (UPNP) client
https://docs.rs/igd/
MIT License
105 stars 41 forks source link

Tokio compatible rust-igd #31

Closed maufl closed 6 years ago

maufl commented 6 years ago

This PR is not finished yet, but I opened it to start discussion about code organization. It is aiming to implement Tokio/Futures compatible functions, as discussed in #30. I feel it's not very nice to have the async functions mixed into the existing modules. I thought about moving them to a new async submodule. I even thought about introduction a new AsyncGateway struct to house all the async gateway functions. What do you think?

sbstp commented 6 years ago

Hey, I'm busy with finals at the moment. I'll check this out once I have a little bit more breathing room.

maufl commented 6 years ago

No problem, I'm still working on it, there are always new issues coming up. I guess it'll be at least another week until it's in a state I can recommend merging.

sbstp commented 6 years ago

Hey, is this ready for review?

maufl commented 6 years ago

Hm, kind of. Everything seems to work but I was not able to use it successfully yet. I always get permission errors when I try to create a port mapping. I'm not sure why. But please take a look and let me know what you think of the way the new code is organized.

sbstp commented 6 years ago

Did the synchronous version work well?

maufl commented 6 years ago

Sorry, I was distracted by other projects. I tried the synchronous version and it didn't work either. I expect it to be a problem with my Fritz!Box. I'll look into it but I don't know when.

povilasb commented 6 years ago

👍 I've been waiting for this :)

maufl commented 6 years ago

I'm currently stuck testing it, my Fritz!Box does not allow adding port mappings, even when I use something like upnpc. I thought I configured it correctly, but something must be wrong.

sbstp commented 6 years ago

This is pretty hard to test. We might as well release this and wait for feedback from the users. @povilasb probably has a large user base to get feedback from.

povilasb commented 6 years ago

Indeed we're using this crate at https://github.com/maidsafe/crust/ which is one of the central libraries for https://safenetwork.org/ . So, yes, we should expect a lot of users. Although, this crate currently is only used in development branch and it might take some time until the masses start using it.

I have a couple of routers on my own, so I can test this PR and give you guys some feedback ;)

povilasb commented 6 years ago

I can confirm that on two routers I have this PR works OK. I've created a simple example that I've tested with: https://github.com/povilasb/rust-igd/blob/async-example/examples/async.rs

After running an example the resulting mappings are

$ igd ls
Description               External Port  Protocol      Internal Port  IP             Status
----------------------  ---------------  ----------  ---------------  -------------  --------
rust-igd-async-example             1234  TCP                    4321  192.168.1.210  Enabled
povilasb commented 6 years ago

👍

sbstp commented 6 years ago

Ready for merge?

povilasb commented 6 years ago

It works on my both routers, so I'd say yes.

sbstp commented 6 years ago

We might want to revisit a few things in the future, such as creating a new tokio Core for every call on the synchronous Gateway and such, but for now this is fine. Great work @maufl .

maufl commented 6 years ago

Make a list, I'd be happy to work on it :)

povilasb commented 6 years ago

I'd say more unit tests too :)