pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
352 stars 115 forks source link

total jobs = 100, too many jobs #29

Closed crylico closed 6 years ago

crylico commented 7 years ago

I'm using the latest commit on master as a submodule in my project.

The program is very simple: it setups up upnp and the web server (I based my xml file off of the examples), then enters a while(1) { ... } where I send a upnp advertisement with timeout INTERVAL, then call sleep(INTERVAL).

Everything works as expected - advertisements + the web server.

After a long period, I get the infamous "total jobs = 100, too many jobs" message. I have compiled with and without UPNP_ENABLE_BLOCKING_TCP_CONNECTIONS defined.

When INTERVAL is large, the messages obviously take longer to show up, but no matter the interval (0, 5, 30, 120, 300), the issue is consistent.

I found this post, but it appears that the codebase has diverged significantly since this time. It seems like the above define was intended to fix the issue, but I'm unsure.

P.S. I love the use of XMACROS for class generation :)

mrjimenez commented 7 years ago

Hi Kyle,

I will take a look at it, but you know, this is a really old bug, it is incredible how it persists. The post you mention is very good, I'll see if this is still the issue.

By the way, xmacros says a lot about how old I am :) I just tried to give the old code some maintainability with a C++ flavor. Glad you liked it, thank you for the kind comment! Someone is actually reading the code :)

Regards, Marcelo.

mrjimenez commented 7 years ago

Hi Kyle,

Indeed, UPNP_ENABLE_BLOCKING_TCP_CONNECTIONS was meant to solve the problem, but notice that even when it is NOT defined, the code allows a blocking connection to happen, in the case that a non-blocking connections fails, take a look at the function private_connect() in upnp/src/genlib/http/httpreadwrite.c:146 and at sock_make_no_blocking() in upnp/src/genlib/net/sock.c:294.

So, the person who originally coded this, knew that there was a possibility for the non-blocking connection to fail and left a fallback, which is something strange, imho, I am having some difficulty in figuring out why this has been done that way.

You should try to see if that is really what is happening, maybe put some printfs there and see if that is the case.

Regards, Marcelo.

crylico commented 7 years ago

hi Marcelo!

thanks for digging into this. i'll check into it and make a pull request if i find success :)

On Apr 15, 2017, 5:36 PM -0400, Marcelo Roberto Jimenez notifications@github.com, wrote:

Hi Kyle,

Indeed, UPNP_ENABLE_BLOCKING_TCP_CONNECTIONS was meant to solve the problem, but notice that even when it is NOT defined, the code allows a blocking connection to happen, in the case that a non-blocking connections fails, take a look at the function private_connect() in upnp/src/genlib/http/httpreadwrite.c:146 and at sock_make_no_blocking() in upnp/src/genlib/net/sock.c:294.

So, the person who originally coded this, knew that there was a possibility for the non-blocking connection to fail and left a fallback, which is something strange, imho, I am having some difficulty in figuring out why this has been done that way.

You should try to see if that is really what is happening, maybe put some printfs there and see if that is the case.

Regards, Marcelo.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (https://github.com/mrjimenez/pupnp/issues/29#issuecomment-294319220), or mute the thread (https://github.com/notifications/unsubscribe-auth/AA-mg8JgZaW_22epKTSMt05D3Dmo81anks5rwThzgaJpZM4MnS-z).

whyman commented 7 years ago

@cweiske is running into this over on https://github.com/v00d00/gerbera/issues/96.

whyman commented 6 years ago

@crylico Can you post a minimal test case that demonstrates the issue?

whyman commented 6 years ago

UpnpSendAdvertisement() is badly named I think - in reality it should be called UpnpAdvertise() as it spawns threads and keeps announcing your device at the interval - as far as I can tell you should not need to call this more than once.

In the loop case above, I imagine you are simply exhausting the Timer thread by adding more than 100 re advertisement jobs?

crylico commented 6 years ago

I removed the loop and only call it once - the advertisements are still valid hours later. I'll keep an eye on sys logs to see if the issue comes up again, but your suggestion seems to make sense. Thanks @v00d00 ! :)

whyman commented 6 years ago

I think we can close this now?

mrjimenez commented 6 years ago

If Kyle agrees, I think we can close it.

Regards, Marcelo.

whyman commented 6 years ago

@crylico can we close this now?

mrjimenez commented 6 years ago

Lets close it, if someone comes up with a problem, either reopen or create another issue.