Clean up Comic class and its connections to QThread

[vedgy]

See the commit messages for details.

[vedgy]

Mac and Windows builds have failed because the ExtractDelegate class in compressed_archive/extract_delegate.h lacks virtual bool isCancelled() = 0; and so can be overriden only if unarr backend is used. Removing override from FileComic::isCancelled() produces a -Winconsistent-missing-override Clang warning on my GNU/Linux system.

I'd like to unify the two extract_delegate.h files into one by adding isCancelled() into compressed_archive/extract_delegate.h (along with a comment that it is only used by unarr backend), removing compressed_archive/unarr/extract_delegate.h and using the same header no matter which compression backend is selected. Perhaps the common extract_delegate.h file should then be moved into another directory.

Is this unification an acceptable/good idea?

[selmf]

After some consideration, I am against merging this PR. This is not because of a quality problem of the new code itself, but because of issues with the comic threading code in general.
To elaborate: Most of the thread code is legacy code and was written for a different C++ standard and with a multithread concept you would not use for new code these days. This goes double for one particular mechanism employed by the comic handling code that I like to call thread ping pong - the code is pushing the threads around like a ping-pong ball between players. While this is kind of stable these days and we roll with it as we don't like to disturb a running system, it clearly is a prime example of an antipattern and it gave us a fair share of trouble. And this is my problem with the provided PR - it enforces the antipattern and the code cleanup makes it look like this is how it is supposed to be.

To conclude: I usually am not a person who dislikes code cleanups or refactoring, but in this case I want the bad code to look as ugly as possible as a deterrent and a sign that deeper refactoring is needed. As much as a cleanup is needed for this, doing it without solving the deeper issues will only serve as spray-coating the rust and hiding the code smell.

@luisangelsm , @vedgy we need to discuss what to do with the leakage fix that is the core intention of this PR

[vedgy]

And this is my problem with the provided PR - it enforces the antipattern and the code cleanup makes it look like this is how it is supposed to be.

If you believe that the code should be reimplemented, a warning comment can be added - perhaps to the new function Comic::moveAndConnectToThread() or its documentation in the header. The good thing is that the code you wish to get rid of is unified. For one thing, the comment can be added into one place instead of 3 places. For another, when the refactoring time comes, all 3 similar places will be more likely taken into account and improved at once.

To conclude: I usually am not a person who dislikes code cleanups or refactoring, but in this case I want the bad code to look as ugly as possible as a deterrent and a sign that deeper refactoring is needed. As much as a cleanup is needed for this, doing it without solving the deeper issues will only serve as spray-coating the rust and hiding the code smell.

I think that a warning comment/documentation is far easier and more explicit than relying on and enforcing the code duplication, ugliness and hoping that it will speed up refactoring. The unification not only improves the code appearance, it also reduces the likelihood of future omissions - when a necessary signal-slot connection is made in one place, but not the others.

[selmf]

Yes, a warning comment would certainly help in this case regardless of the state of the code cleanup. There is another reason why I am not keen on merging this in its current state - testing.

The code cleanup touches several sections of the code unrelated to the leak in ComicController and even if the modifications seem safe there is a high risk of side effects, exposing of previously masked errors and the general amount of testing needed to approve this is very high as we need to verify the proper threading in all three binaries YACReader ships, on all platforms.

I don't know what @luisangelsm opinion on this is but personally I would prefer merging the changes proposed in this PR as separate easily testable steps, i.e. one PR with minimal impact to fix the thread leak followed by some safe code cleanup.

[vedgy]

I think the fix itself is the most risky part of this pull request. All other changes practically obviously don't affect the application behavior. Saving on testing time is exactly why I prefer to bundle related code cleanups with code fixes. Usually as separate commits within a single pull request. This way everything can be tested in one step rather than separately.

Another reason I avoid separate pull requests is conflicts. When everything is separate, either merge conflicts have to be resolved (which risks introducing a bug in a botched merge) or I have to wait for one pull request to be merged before creating another one (which is inconvenient in view of review times that are often quite long here).

Admittedly, the fix is not in a separate commit here. I can split the fix into a separate commit or even into another pull request depending on @luisangelsm's preference.

[vedgy]

When I was implementing this function, I thought that with the default connection type Qt::AutoConnection, the actual connection type is determined statically inside the QObject::connect() call. But after some debugging, reading Qt source code and rereading the documentation, I realized that this is a truly different connection type:

The connection type is determined when the signal is emitted.

Since both ComicController classes connected to all signals using Qt::AutoConnection, calling this function instead changes their behavior. If this change is undesirable, I can restore the use of Qt::AutoConnection by passing one more argument to this function - qthreadQuitConnectionType.

[luisangelsm]

I think we don't want Qt::QueuedConnection in the server side. Do you use YACReader for iOS? Are these changes tested with it?

[vedgy]

No, I don't use or test the server. I have noticed the leak fixed in this PR while looking at code patterns similar to the one fixed in #203 and comparing very similar Render and ComicController* connection code. Then decided to unify the code to prevent future accidental divergence.

[vedgy]

Now I see that there is no real leak in the current implementation as Server never calls Comic::invalidate(). Though it might call it in the future and connecting to Comic::invalidated still make sense.

So it turns out that this pull request is pure code refactoring, not a fix.

[selmf]

If there is no real leak to be fixed here I personally would prefer to leave the code as it is. It is easy to introduce unwanted errors and hard to catch undefined behavior when working with QThread and the code is basically in maintenance mode already, so there is very little risk of future accidental divergence.

Again, this is just my opinion, but I am actually for future divergence in the code. In its current state it is a dead end and the best way forward is to extract and refactor the core functionality to a new structure to which the different parts of the YACReader suite can then be ported step by step while keeping the old structure as a fallback as long as it is needed.

[selmf]

One more thing. If we merge this the connection instructions for the slots and signals will no longer happen on the main thread but on the comic thread. That is a major change from the current behavior and it goes beyond the scope of a simple code cleanup.

[vedgy]

One more thing. If we merge this the connection instructions for the slots and signals will no longer happen on the main thread but on the comic thread.

Why? A simple function call cannot magically be transferred to another thread.

In its current state it is a dead end and the best way forward is to extract and refactor the core functionality to a new structure to which the different parts of the YACReader suite can then be ported step by step while keeping the old structure as a fallback as long as it is needed.

Apart from the thread ping pong and the QCoreApplication::sendPostedEvents() hack introduced in #220, what are the major issues with Comic that justify rewriting it before fixing other legacy code issues? The new implementation would certainly take a lot of time to implement and test. The only other benefit of the rewriting that you have mentioned is: a single thread would be reused for reading all comic files. This performance improvement doesn't seem very important to me. Switching to a different comic is normally not frequent enough to make creating a new thread into a serious performance issue. And not sharing a thread probably allows for simpler code with less potential for races.

So an alternative to rewriting Comic in the nearest future is to apply the cleanup in this pull request, implement a rather simple proxy object alternative to the temporary solution implemented in #220 and eliminate the thread ping pong. This can realistically be done in YACReader 9.9.0. Unless there is some other fundamental issue with the current Comic implementation I am not aware of.

One more potential Server issue I just noticed: ~YACReaderHttpSession() simply deletes its comic and remoteComic. Unless some other code ensures that Comic::process() ends before this destructor is called, the Server could crash. Though the code seems to indicate that ~YACReaderHttpSession() itself is never called. Some refactoring of the Server code might be more urgent than rewriting the Comic class :)

[selmf]

Why? A simple function call cannot magically be transferred to another thread.

https://doc.qt.io/qt-5/qobject.html#moveToThread
https://doc.qt.io/qt-5/qobject.html#connect-2

Sorry, but I don't have the time for a lengthy discussion on this. This is more than a cosmetic code change and I'd rather spend my time on getting actual work done instead of defending my decisions on this matter because you expect me to back up every choice I make with a quote. I have better things to do.

[vedgy]

Either I misunderstand your statement or you are mistaken. The connect statements inside Comic::moveAndConnectToThread() cannot possibly execute in the new thread. The new thread isn't even started yet when this function is called. Just inserted qCritical() << "Comic::moveAndConnectToThread()" << QThread::currentThread(); before and after the call to moveAndConnectToThread() in Render::startLoad(), before and after the moveToThread() call inside moveAndConnectToThread() and at the end of moveAndConnectToThread(). As expected, all printed pointers are equal. Even for different comic files. The connections happen in the same Render's thread as before this pull request.