search

sbstp / rust-igd

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

Problems with signal traps since 0.7 #36

Closed dswd closed 5 years ago

dswd commented 5 years ago

I am the author of vpncloud (https://github.com/dswd/vpncloud) and I am using the igd crate to establish port forwardings in NAT routers. Many thanks for the great crate by the way. Since version 0.7 I experience the problem, that the vpncloud binary can't be closed via ctrl-c as my custom signal traps do not work anymore. It seems that since 0.7 something swallows all signals.

If you want to test it, you can use my current code, compile the binary and run vpncloud --type dummy. If you upgrade idg to 0.7 ctrl-c will stop working.

Do you have any idea on how to prevent this? Any help will be greatly appreciated.

sbstp commented 5 years ago

It might be related to tokio. Even though we don't use signals explicitly in the crate it might be something that tokio is doing to handle the shutdown of the loop. In 0.7 the synchronous code uses the async code and just blocks on the future.

Does the problem happen systematically or does it only happen while you are doing port mapping? If there's a tokio thread pool somehow left after we do the port mapping, it's probably an issue.

dswd commented 5 years ago

When port forwarding is enabled in vpncloud, crtl-c does no longer work at all. The process just keeps on running infinitely. It is also not the case that something blocks the process from exiting (there is a "shutting down..." message right after receiving the signal that is not printed, so the signal is never received). When I disable the port forwarding functionality with commandline arguments, ctrl-c works reliably. So the problem must be related to actually calling into igd/tokio and not just including it. The problem does not seem to be related to whether port forwardings are requested while pressing ctrl-c. The problem persists over multiple minutes and repeated presses of ctrl-c and kill commands. Only kill -9 helps.

sbstp commented 5 years ago

Alright, thanks for the info. I'm trying to refactor the library right now and I will update all the crates and remove the async code from the sync methods calls. Hopefully that will help you.

dswd commented 5 years ago

Thank you very much.

dswd commented 5 years ago

Oh, when you refactor the library, could you make the tokio/async parts optional using a feature? Currently igd pulls in lots of dependencies, that I think are not needed for my use case.

sbstp commented 5 years ago

Yes, I was planning on putting the async stuff behind a feature to make it more lightweight, but I was also planning on using reqwest for the HTTP needs and reqwest depends on the tokio stack. In fact right now it uses hyper which also depends on the tokio stack.

I've been working on a small, lightweight HTTP client in my free time but I'm not quite sure if its robust enough to be used in this crate yet. https://github.com/sbstp/lynx

dswd commented 5 years ago

Oh that sounds interesting. However you are facing a small name clash: here https://lynx.browser.org/

sbstp commented 5 years ago

Hey, I just pushed my refactor to the master branch. Can you tell me if your bug is fixed? You can use a git dependency temporarily before I publish this. 

igd = { git = "https://github.com/sbstp/rust-igd.git" }
dswd commented 5 years ago

Ok I tested your version: The good thing is that ctrl-c works now. The bad thing is that port forwarding does not work anymore: 

ERROR - Port-forwarding: failed to obtain external IP: Request Error. Invalid response from gateway: <HTML><HEAD><TITLE>500 Internal Server Error (ERR_INVALID_REQ)</TITLE></HEAD><BODY><H1>500 Internal Server Error</H1><BR>ERR_INVALID_REQ<HR><B>Webserver</B> Sun, 03 Mar 2019 09:19:16 GMT</BODY></HTML>
dswd commented 5 years ago

So I did some debugging with wireshark. The differences between your invalid lynx requests and the valid hyper requests are the following:

The headers are quite different:

The body looks roughly the same with the sole exception that hyper seems to indent the SOAP-ENV tag by 8 spaces while lynx does not.

If I were to bet, I would put my money on the Content-Type header.

sbstp commented 5 years ago

Alright I've updated lynx to add the Host header and updated this crate with the Content-Type header. Let me know if this works.

dswd commented 5 years ago

Yes, it works. Thanks a lot for your immediate help. Please tell me when you released those fixes.

sbstp commented 5 years ago

Alright, it's released in 0.8.0.

dswd commented 5 years ago

Thanks. Do you actually need openssl as a dependency or is that a mistake?

sbstp commented 5 years ago

I've updated igd with the new version of lynx and disabled the features. I've also removed the regex crate, it wasn't needed. Can you test the crate again and I'll publish it if all goes well?

Here's the new dependency tree, it's pretty much as light as it's going to get.

igd v0.8.0 (/home/simon/projects/rust-igd)                                                                                                                                                                                                    
├── lynx v0.1.2
│   ├── http v0.1.16
│   │   ├── bytes v0.4.11
│   │   │   ├── byteorder v1.3.1
│   │   │   └── iovec v0.1.2
│   │   │       └── libc v0.2.48
│   │   ├── fnv v1.0.6
│   │   └── itoa v0.4.3
│   ├── log v0.4.6
│   │   └── cfg-if v0.1.6
│   └── url v1.7.2
│       ├── idna v0.1.5
│       │   ├── matches v0.1.8
│       │   ├── unicode-bidi v0.3.4
│       │   │   └── matches v0.1.8 (*)
│       │   └── unicode-normalization v0.1.8
│       │       └── smallvec v0.6.8
│       │           └── unreachable v1.0.0
│       │               └── void v1.0.2
│       ├── matches v0.1.8 (*)
│       └── percent-encoding v1.0.1
├── rand v0.4.6
│   └── libc v0.2.48 (*)
├── url v1.7.2 (*)
└── xmltree v0.8.0
    └── xml-rs v0.7.0
        └── bitflags v1.0.4
dswd commented 5 years ago

Wow, it works and that update reduced my binary size by 25%.