Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't decompress the reply data for Qt 6.3 #17133

Merged
merged 1 commit into from
May 31, 2022

Conversation

brvphoenix
Copy link
Contributor

Backport #17120.

@brvphoenix brvphoenix changed the title [backport] Don't decompress the reply data for Qt 6.3 Don't decompress the reply data for Qt 6.3 May 30, 2022
@xavier2k6 xavier2k6 added this to the 4.4.4 milestone May 30, 2022
@@ -121,9 +124,13 @@ void DownloadHandlerImpl::processFinishedDownload()
}

// Success
#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
m_result.data = m_reply->readAll();
#else
m_result.data = (m_reply->rawHeader("Content-Encoding") == "gzip")
? Utils::Gzip::decompress(m_reply->readAll())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could detect whether the data is really gzipped in Utils::Gzip::decompress().

Comment on lines +127 to +133
#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
m_result.data = m_reply->readAll();
#else
m_result.data = (m_reply->rawHeader("Content-Encoding") == "gzip")
? Utils::Gzip::decompress(m_reply->readAll())
: m_reply->readAll();
#endif
Copy link
Contributor Author

@brvphoenix brvphoenix May 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
m_result.data = m_reply->readAll();
#else
m_result.data = (m_reply->rawHeader("Content-Encoding") == "gzip")
? Utils::Gzip::decompress(m_reply->readAll())
: m_reply->readAll();
#endif
auto rawData = m_reply->readAll();
m_result.data = (m_reply->rawHeader("Content-Encoding") == "gzip" && rawData.at(0) == 0x1f && static_cast<unsigned char>(rawData.at(1)) == 0x8b)
? Utils::Gzip::decompress(rawData)
: rawData;

@glassez
Find some reference from here https://stackoverflow.com/questions/6059302/how-to-check-if-a-file-is-gzip-compressed.

Test with Qt 6.3 and it works. But I don't know whether it will work for Qt < 6.3. Besides, I think this method is somewhat hacky.

Update: The suggested change also works with Qt 5.15.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, I think this method is somewhat hacky.

Is this some kind of documented watermark of the Gzip format that you use in your suggestion above? If Yes, I wouldn't call this method hacky.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think compressed file and compress method should be known from content-encoding header.

Because Qt also support Brotli (known as content-encoding: br header): https://doc.qt.io/qt-5/qtwebengine-3rdparty-brotli.html.

Maybe we should also handle Brotli ?

Copy link
Contributor Author

@brvphoenix brvphoenix May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this some kind of documented watermark of the Gzip format that you use in your suggestion above?

From here: https://www.ietf.org/rfc/rfc1952.txt


      2.3.1. Member header and trailer

         ID1 (IDentification 1)
         ID2 (IDentification 2)
            These have the fixed values ID1 = 31 (0x1f, \037), ID2 = 139
            (0x8b, \213), to identify the file as being in gzip format.

         CM (Compression Method)
            This identifies the compression method used in the file.  CM
            = 0-7 are reserved.  CM = 8 denotes the "deflate"
            compression method, which is the one customarily used by
            gzip and which is documented elsewhere.

Maybe we should also handle Brotli ?

request.setRawHeader("Accept-Encoding", "gzip");


@glassez @Chocobo1

Do we really need to address this situation ourself instead of relying on upstream Qt? I'm open to this because we only accept the GZip encoding after all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to address this situation ourself instead of relying on upstream Qt?

I would prefer to rely on Qt. However it is strange that Qt 6.3 made a (big?) breaking change and it isn't mentioned on their change log.

Besides, I think this method is somewhat hacky.

I thought so too, detection at this location could have false positives AFAIK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However it is strange that Qt 6.3 made a (big?) breaking change and it isn't mentioned on their change log.

That's exactly what I'm worried about. But let's leave it as it is for now and see what happens next.

@Chocobo1 Chocobo1 merged commit af07a98 into qbittorrent:v4_4_x May 31, 2022
@Chocobo1
Copy link
Member

@brvphoenix
Thank you!

userdocs added a commit to userdocs/qbittorrent-nox-static that referenced this pull request Jun 1, 2022
@brvphoenix brvphoenix deleted the v4_4_x branch June 11, 2022 13:29
@brvphoenix brvphoenix mentioned this pull request Jul 26, 2022
@HoundCat
Copy link

I've been patiently waiting for a fix for this issue, which fails to update RSS feeds, of over a year but being patient for that long is far above and beyond the length of time it should take to wait for a fix to a major function in any program (the first software company I worked for tanked, being purchased by their competition as a result of delaying their dBASE release by a year, and that was when releases received fixes before the Internet only once or twice each year - the COMDEX days), or should I concede that this issue is not important enough to fix, and it would be best to go back to bittorrent/utorrent for Windows and Mac, and find something else for my Linux platforms? If that's the case, I have no problem switching torrent client applications from another developer who implements their fixes rather than posting and closing bug reports without fixing issues. I switched from the definitive torrent client(s) because of qbittorrent's greater cross platform compatibility (I have Windows, MacOS, Android, Fedora, and Ubuntu Linux clients, as well as physical clients to whom I've recommended and installed qbittorrent, which will require upwards of weeks to migrate -- not something I wish to do, but waiting for software fixes for more than a year stretches confidence in my recommendations and the loyalty behind the recommendations I make, reflecting back to the perception that customer's issues aren't important enough to resolve, and therefore, neither are the customers. Switching software I use and recommend, though a huge task, is preferable to waiting and exhausting resources in efforts to find solutions to problems that aren't high enough priority to the development company to resolve. Searching for a solution takes time and has all resulted in closed reports without resolutions, only pointing to other reports with the same closures or redundant circular report reference. Switching would actually save all this time, including very wordy queries such as this, which is as much a head's up to you guys that you're about to lose supporting techs, their customers, and others to whom they recommend when they're happy, as it is a plea to resolve this issue for the people who do stick around. Better yet, if RSS feeds isn't a priority, removing it would eliminate any and all problem reports related - impact would be less functionality but allow greater priority to be placed on the limited features that remain. Frustrated ranting, I know, but what else might make this a priority or a recommendation to abandon ship? More waiting? Enough already!

@glassez
Copy link
Member

glassez commented Mar 15, 2023

@HoundCat
Sorry, I don't speak English well enough to read comments like yours above. If I understood its essence correctly, then you are experiencing some problems with updating the RSS, which for some reason you associate with a problem that was fixed a year ago with this patch.
Note that the problems of updating the RSS can occur due to various reasons. So if I were you, I would just post a separate Issue with the details of exactly your problem and all the other data that could be useful to understand what is really going on in your case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants