Disable processing socket events in findIncompleteFiles()

[Chocobo1]

Related to bda5be8, but this time is from webUI.

[zeule]

Could you explain, please, why this is needed? Don't understand neither this change nor the mentioned bda5be8.

[Chocobo1]

Could you explain, please, why this is needed? Don't understand neither this change nor the mentioned bda5be8.

You will need to see the crash report in #7327.
findIncompleteFiles() was called many times before the previous instance can complete and since we have bda5be8, it should make sense to make this change. Also in #7436, the op says it was crashing when adding torrent via webUI.
Said that much, however, I still cannot pinpoint the root cause of crash, only assuming this change could alleviate the problem.

[zeule]

Thank you. Don't understand anyway why and how findIncompleteFiles() was called repeatedly. Could you explain, please?

[Chocobo1]

Don't understand anyway why and how findIncompleteFiles() was called repeatedly. Could you explain, please?

due to qApp->processEvents(). From what I know, users can add torrents via webUI fairly quick.

[zeule]

I still can't make any sense out of it :(. Why did it crash? What are the other kind of events that you want to process in the loop inside findIncompleteFiles()?

[Chocobo1]

I still can't make any sense out of it :(. Why did it crash?

Thread safety issues? ¯\(ツ)/¯

What are the other kind of events that you want to process in the loop inside findIncompleteFiles()?

No solid idea either, although I suspect some GUI painting events to make qbt appear active.

[zeule]

No solid idea either, although I suspect some GUI painting events to make qbt appear active.

When web UI and GUI are used simultaneously?

@glassez, could you tell us, why did it crash and why it was fixed by bda5be8?

[glassez]

@glassez, could you tell us, why did it crash and why it was fixed by bda5be8?

Ok. Let's see the following sequence:

1. User add torrent via dialog and press Ok button
2. AddNewTorrentDialog::accept() is called and it reaches BitTorrent::Session::addTorrent()
3. BitTorrent::Session::addTorrent() is called and it reaches BitTorrent::Session::findIncompleteFiles()
4. If torrent has many files then QCoreApplication::processEvents() can be called and it can process another "OK button click" event (if user click OK one more time for any reason)
5. Steps 2-4 can be repeated for each additional button click
6. When most inner event returns from BitTorrent::Session::addTorrent() it reaches QDialog::accept() that (in the pass) destroys the dialog itself
7. The next level event returns from BitTorrent::Session::addTorrent() into method of already destroyed object and voila!

Something like this... Maybe I missed some details, or mixed up something, but the GENERAL MEANING should be clear.

[zeule]

Thanks for the explanations! Just one more question: why can't this be solved by disabling the dialog in AddNewTorrentDialog::accept()? Additionally the user would get visual response from the button click,

[glassez]

Additionally the user would get visual response from the button click

This can be a good idea in itself. But we cannot ensure that more than one "button click" event will not appear in the queue before the first of them will begin to be processed.

[Chocobo1]

Now I've a new view on the stack dump in #7436.

It's happening a lot in these few lines I believe:

QIODevice::write()    // pointC
Http::Connection::sendResponse(response)[ base\http\connection.cpp : 93 ]  // pointB
Http::Connection::read()[ base\http\connection.cpp : 85 ]  // pointA

Some background: A webUI socket connect will disconnect after 7 sec of idleness, see: https://github.com/qbittorrent/qBittorrent/blob/master/src/base/http/server.cpp#L64

• At pointA (time==0), qbt is receiving a new request (specifically action_command_download()), this for unknown reason (accessing network drive?) is taking a lot of time (say 10sec), and then it hits qApp->processEvents().
• At time==7, the socket timeout occurs, it proceeds to erase all traces of it.
• At time==10, the action_command_download() has completed, it continues to reply (pointB) its result to the client, but the resources was removed at time==7, thus at pointC it is a use-after-free sigfault.

Now I think adding QEventLoop::ExcludeSocketNotifiers flag won't solve anything, because the socket timeout is still being processed by the event loop.
The only solution is removing qApp->processEvents() here. I never liked its place at here, it reorders events which should be blocking.

[glassez]

The only solution is removing qApp->processEvents() here. I never liked its place at here, it reorders events which should be blocking.

I don't like processEvents here too. The only its goal is to prevent GUI freezing when user adding torrent with many files.

[Chocobo1]

I don't like processEvents here too. The only its goal is to prevent GUI freezing when user adding torrent with many files.

Or maybe enable processEvents only when webUI isn't used:

if (!webUIEnabled())
  qApp->processEvents(QEventLoop::ExcludeUserInputEvents);

[Chocobo1]

PR updated.

[zeule]

sEgfault

[Chocobo1]

Also fixed in commit msg, thank you.

[glassez]

Or maybe enable processEvents only when webUI isn't used:

I think we should get rid of it at all. This can be a source of some hard to find errors. E.g. when we run the application, we must first renew all the existing torrents, and nothing should interfere in this process. How can we ensure this? Besides, it affects only torrents with really many files (I think they should be in the thousands).

[glassez]

And yet... Your solution creates an unacceptable dependency (core shouldn't be dependent from the user interface).

[Chocobo1]

I think we should get rid of it at all.

OK, PR updated.

[Chocobo1]

If no one objects it, I'll merge it after waiting 24hrs.

[zeule]

Just a question: why nobody discusses a multi-threaded approach?

[glassez]

I thought about it. Maybe later, unless someone want to do it too.

[Chocobo1]

Just a question: why nobody discusses a multi-threaded approach?

Session::findIncompleteFiles() looks like IO bound to me.

[Chocobo1]

Everyone thank you!