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

Fix server crash (unreported) #47271

Merged
merged 1 commit into from
Feb 9, 2022
Merged

Fix server crash (unreported) #47271

merged 1 commit into from
Feb 9, 2022

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Feb 9, 2022

Followup #47029

@elpaso elpaso added Regression Something which used to work, but doesn't anymore Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption Server Related to QGIS server labels Feb 9, 2022
@github-actions github-actions bot added this to the 3.24.0 milestone Feb 9, 2022
@pblottiere
Copy link
Member

Argh, I added a lot of unit tests but it wasn't enough... Can you add in these tests the filter string which is causing the issue please?

@elpaso
Copy link
Contributor Author

elpaso commented Feb 9, 2022

Argh, I added a lot of unit tests but it wasn't enough... Can you add in these tests the filter string which is causing the issue please?

I guess it just happens when there are no filters.

@pblottiere
Copy link
Member

I guess it just happens when there are no filters.

Strange, there's already a test with an empty string filter. But the fix is still valid :).

@rldhont
Copy link
Contributor

rldhont commented Feb 9, 2022

backport 3.22 ?

@elpaso
Copy link
Contributor Author

elpaso commented Feb 9, 2022

backport 3.22 ?

I wasn't sure if #47029 landed in 3.22, if yes I think this needs backporting.

@elpaso
Copy link
Contributor Author

elpaso commented Feb 9, 2022

I guess it just happens when there are no filters.

Strange, there's already a test with an empty string filter. But the fix is still valid :).

Can you point me to the test? I'll run the debugger onto it.

@pblottiere
Copy link
Member

Can you point me to the test? I'll run the debugger onto it.

https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgsserver.py#L533-L538

@elpaso
Copy link
Contributor Author

elpaso commented Feb 9, 2022

Can you point me to the test? I'll run the debugger onto it.

https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgsserver.py#L533-L538

That test crashes on my local machine without this patch. It hits qstring.h:

.ASSERT: "uint(i) < uint(size())" in file /home/xxx/Qt/5.15.2/gcc_64/include/QtCore/qstring.h, line 1072
inline const QChar QString::at(int i) const
{ Q_ASSERT(uint(i) < uint(size())); return QChar(d->data()[i]); }

So it was basically already covered by tests, I wonder why it didn't fail on CI, it is probably because QT_NO_DEBUG was used for QT builds used on CI.

@elpaso elpaso merged commit 4171ec9 into qgis:master Feb 9, 2022
@qgis-bot
Copy link
Collaborator

qgis-bot commented Feb 9, 2022

The backport to release-3_22 failed:

The process '/usr/bin/git' failed with exit code 1
stderr
error: could not apply 47d047ea31... Fix server crash (unreported)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout
Auto-merging src/server/qgsserverparameters.cpp
CONFLICT (content): Merge conflict in src/server/qgsserverparameters.cpp

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_22 release-3_22
# Navigate to the new working tree
cd .worktrees/backport-release-3_22
# Create a new branch
git switch --create backport-47271-to-release-3_22
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 47d047ea313e0af89c686b634513deaec36d4c75
# Push it to GitHub
git push --set-upstream origin backport-47271-to-release-3_22
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_22

Then, create a pull request where the base branch is release-3_22 and the compare/head branch is backport-47271-to-release-3_22.

@qgis-bot qgis-bot added the failed backport The automated backport attempt failed, needs a manual backport label Feb 9, 2022
@elpaso elpaso deleted the fix-server-crash branch February 23, 2022 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption failed backport The automated backport attempt failed, needs a manual backport Regression Something which used to work, but doesn't anymore Server Related to QGIS server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants