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

[Postgres] Remove listener wait condition #55062

Merged
merged 1 commit into from Feb 28, 2024

Conversation

troopa81
Copy link
Contributor

Fixes #54260

The issue was because of 2 things:

  • credential were requested though it was not necessary (auth configuration needed to be extended)
  • Display the credential dialog was not possible because of the main thread blocking waitCondition

This PR propose to create the connection and execute the listen request in the main thread and then doing the listening on the thread. This way, we still can garantee listening is set when setListening() ends and not block UI if credentials dialog need to popup.

I also reworked code a bit to use a plain QgsPostgresConn and log the listen connection.

@troopa81 troopa81 added Bug Either a bug report, or a bug fix. Let's hope for the latter! PostGIS data provider backport queued_ltr_backports Queued Backports labels Oct 26, 2023
@github-actions github-actions bot added this to the 3.34.0 milestone Oct 26, 2023
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 10, 2023
@troopa81 troopa81 removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 10, 2023
@troopa81
Copy link
Contributor Author

@elpaso Do you mind reviewing this PR please ?

Copy link

github-actions bot commented Dec 5, 2023

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 5, 2023
@troopa81 troopa81 removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 6, 2023
@troopa81
Copy link
Contributor Author

troopa81 commented Dec 6, 2023

@strk Do you mind reviewing this PR ?

@strk
Copy link
Contributor

strk commented Dec 14, 2023

Had you tried adding a testcase showing the crash ? I'd start there. I see fixes but I would prefer to see the crash first, and doing it with a test ensures it won't happen again in the future.

@troopa81
Copy link
Contributor Author

Had you tried adding a testcase showing the crash ? I'd start there. I see fixes but I would prefer to see the crash first, and doing it with a test ensures it won't happen again in the future.

You can do what is described in #54260 but you have to have a configuration authentication for the database layer. That's the main reason of the crash.

@strk
Copy link
Contributor

strk commented Dec 19, 2023 via email

@troopa81
Copy link
Contributor Author

Can you try to write such test ?

I thought about it but there is thread and UI synchronization involved, it's a mess to test and would be very hard to make a non-flaky test about it

Copy link

github-actions bot commented Jan 3, 2024

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 3, 2024
@troopa81 troopa81 removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 3, 2024
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 18, 2024
@troopa81 troopa81 removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 18, 2024
Copy link

github-actions bot commented Feb 2, 2024

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 2, 2024
Copy link

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Feb 10, 2024
@troopa81 troopa81 removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 12, 2024
@troopa81 troopa81 reopened this Feb 12, 2024
@troopa81 troopa81 closed this Feb 26, 2024
@troopa81 troopa81 reopened this Feb 26, 2024
Copy link

github-actions bot commented Feb 26, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit b20a5d0)

@troopa81 troopa81 merged commit a98a8ec into qgis:master Feb 28, 2024
29 checks passed
@troopa81
Copy link
Contributor Author

Thanks for the review @elpaso

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter! PostGIS data provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activating notify/listen feature crash QGIS
3 participants