Fix hanging connections because maxPendingConnections is reached#1749
Conversation
Since QLC+ doesn't use pending connections (there is no call to nextPendingConnection anywhere), it is no problem to remove support for that. Either that, or we should call nextPendingConnection() for every connection we add with addPendingConnection(QTcpSocket*)
|
Interesting one!
An alternative is Qt HTTP Server, but they licensed it as GPLv3 so not compatibe with QLC+ code |
Could very well be. The forum message talks about two different clients, each with a browser. That's two times the connection count of a single user. When a single user "behaves like a normal user" and doesn't CTRL+SHIFT+R like a madman, you probably don't notice. But double that count, and it might be enough to trigger the bug. I also think the bug is more prevalent now that the favicons are in. The browser will try to side-load these, using yet another connection. On a side note: I do notice this bug regularly when doing shows. My use-case is that I'm on an Android tablet, switching between another app and the browser. The browser will reload every time I switch. That has probably triggered this bug. When it occurs I notice that QLC+ will still run the light show and even respond to OSC messages. So the app in general is not crashing, just the HTTP web console side. And a stop+start of the QLC+ process fixes everything.
As explained here. I see... Too bad 🙁 |
this change doesn't change anything, because m_request is always created by passing QHttpConnection as its parent. Both are QObjects, so when QHttpConnection is destructed, QHttpRequest is deleted as well because it is a child of qhttpconnection (in QObject terms). On the countrary, this change is harmful because deleting QObjects directly via delete operator instead of calling deleteLater can cause a crash, because deleteLater proceeds with deletion only after all signal-slot connections have been processed. If QObject is deleted via delete operator and there are still signal-slot connections pending, they will operate on the already freed pointer. |
|
I have tried this branch and it works well, I am voting for merging it |
So is this change good or bad? Didn't seem so from your first review |
|
This QLC+ PR is good. I was referring to qhttpserver PR nikhilm/qhttpserver#64 which we should not port to our code. |
Ah OK! |
4 participants
The bug in question:
Open QLC+ using
--web, start a browser and navigate tohttp://localhost:9999, then start refreshing bypassing the browsers cache using CTRL+SHIFT+R. After some attempts you will see that the browser's connection starts hanging.The reason for this is that every connection is added to the list of "pending connections", and this list is finite. By default, it's 30 positions. So after 30 connections (with every page request using multiple connections), things get clogged up.
The solution:
Since QLC+ doesn't use pending connections in
QHttpServer(there is no call tom_tcpServer->nextPendingConnection()), it is no problem to remove support for that.Either that, or we should call nextPendingConnection() for every connection we add with addPendingConnection(QTcpSocket*). But I wasn't sure where to do that call.
How I tested:
I restarted the browser, bashed CTRL+SHIFT+R again, and noticed I couldn't get the browser connections to hang. Also, I tested that websockets still work by sliding the GM and clicking a button, both in the virtual console and the web console.
On a side note, the QHttpServer project seems abandoned. There is one PR still waiting, which seems to fix a memory leak. Maybe that PR should be applied in QLC+? Or maybe there is a maintained alternative to QHttpServer somewhere?