Use QueuedConnection for signals in QCoroNetworkReply#245
Merged
Conversation
26f8d75 to
ece2678
Compare
The common pattern (even the one shown in our docs) of using QCoro with QNetworkReply is a local stack-allocated QNAM, making a request and then co_awaiting on the returned QNetworkReply. Once the coroutine is resumed, it likely returns, destroying the QNAM with it. The problem is, that after the coroutine returns, the execution returns back into the code inside QNetworkReply that emitted the signal, which then leads to invalid memory access, since the code has likely been already destroyed alongside the QNAM. Surprisingly, on Linux this does not crash and even ASAN does not detect it (would likely need to compile Qt with ASAN), but Valgrind was able to catch the invalid memory access inside Qt. On Windows this just crashes. The solution is to use QueuedConnection between QNetworkReply and QCoro, so that we can be sure that by the time our code is called (leading to the destruction of QNAM), no code in the QNAM or its associated classes is being executed, so the destruction is clear. I had to resort to some hackery to keep the QCoro struct binary compatible, so more cruft to be removed in 1.0. Fixes #231
ece2678 to
513723d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The common pattern (even the one shown in our docs) of using QCoro with QNetworkReply is a local stack-allocated QNAM, making a request and then co_awaiting on the returned QNetworkReply. Once the coroutine is resumed, it likely returns, destroying the QNAM with it. The problem is, that after the coroutine returns, the execution returns back into the code inside QNetworkReply that emitted the signal, which then leads to invalid memory access, since the code has likely been already destroyed alongside the QNAM. Surprisingly, on Linux this does not crash and even ASAN does not detect it (would likely need to compile Qt with ASAN), but Valgrind was able to catch the invalid memory access inside Qt. On Windows this just crashes.
The solution is to use QueuedConnection between QNetworkReply and QCoro, so that we can be sure that by the time our code is called (leading to the destruction of QNAM), no code in the QNAM or its associated classes is being executed, so the destruction is clear.
I had to resort to some hackery to keep the QCoro struct binary compatible, so more cruft to be removed in 1.0.
Fixes #231