qt: Remove redundant stopThread() and stopExecutor() signals #14250

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:stopthread-signal changing 6 files +7 −16
  1. hebasto commented at 10:29 PM on September 17, 2018: member

    The QThread::finished signal do this work.

  2. fanquake added the label GUI on Sep 17, 2018
  3. in src/qt/bitcoin.cpp:282 in 041be348b0 outdated 
     377 | @@ -379,8 +378,7 @@ void BitcoinApplication::startThread()
 378 |      connect(this, &BitcoinApplication::requestedInitialize, executor, &BitcoinCore::initialize);
 379 |      connect(this, &BitcoinApplication::requestedShutdown, executor, &BitcoinCore::shutdown);
 380 |      /*  make sure executor object is deleted in its own thread */
 381 | -    connect(this, &BitcoinApplication::stopThread, executor, &QObject::deleteLater);
 382 | -    connect(this, &BitcoinApplication::stopThread, coreThread, &QThread::quit);
 383 | +    connect(coreThread, &QThread::finished, executor, &QObject::deleteLater);
    promag commented at 12:22 AM on September 18, 2018:

    How is deleteLater called if the coreThread event loop exited?

    hebasto commented at 6:37 AM on September 18, 2018:

    @promag https://doc.qt.io/qt-5.9/qthread.html:

    From Qt 4.8 onwards, it is possible to deallocate objects that live in a thread that has just ended, by connecting the finished() signal to QObject::deleteLater().

    https://doc.qt.io/qt-5.9/qthread.html#finished

    promag commented at 6:46 AM on September 18, 2018:

    TIL.

  4. DrahtBot commented at 2:13 AM on September 18, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #13674 (Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) by LeandroRocha84)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. fanquake requested review from jonasschnelli on Sep 18, 2018
  6. jonasschnelli commented at 9:43 PM on September 20, 2018: contributor

    seems more elegant, utACK 041be348b0bd96d87eb3d1a2b0edc47538dc9f66 will test soon.

  7. DrahtBot cross-referenced this on Sep 21, 2018 from issue Add BitcoinApplication & RPCConsole tests by ryanofsky
  8. jonasschnelli commented at 11:59 AM on September 25, 2018: contributor

    Tested ACK #041be348b0bd96d87eb3d1a2b0edc47538dc9f66 https://bitcoin.jonasschnelli.ch/build/795

  9. jonasschnelli removed review request from jonasschnelli on Sep 25, 2018
  10. DrahtBot commented at 7:53 AM on September 28, 2018: contributor

    <!--32850dd3fdea838b4049e64f46995ea2-->

    Coverage Change (pull 14250) Reference (master)
    Lines +0.0044 % 87.0361 %
    Functions +0.0154 % 84.1130 %
    Branches -0.0038 % 51.5451 %
  11. hebasto commented at 3:07 PM on October 12, 2018: member

    @Sjors Could you possibly to review this PR?

  12. Sjors commented at 7:21 AM on October 13, 2018: member

    Definitely looks cleaner, but I'm not sure what to test: quitting the app still works on macOS 10.14 in 041be348b0bd96d87eb3d1a2b0edc47538dc9f66, so that's good.

  13. jonasschnelli commented at 6:53 PM on October 17, 2018: contributor

    Needs more tests (@Sjors, @fanquake, etc.)...

  14. fanquake commented at 1:29 AM on October 18, 2018: member

    tACK 041be34

    Tested on top of 816fab9. If anyone has extra tests etc that could be run against this please post them here.

  15. fanquake requested review from laanwj on Oct 18, 2018
  16. fanquake added this to the "Mergeable" column in a project
  17. hebasto renamed this:
    qt: Remove redundant stopThread() signal
    qt: Remove redundant stopThread() and stopExecutor() signals
    on Oct 21, 2018
  18. hebasto commented at 12:33 PM on October 21, 2018: member

    Added similar cleanup for RPCExecutor. Please re-review.

  19. fanquake removed this from the "Mergeable" column in a project
  20. DrahtBot cross-referenced this on Oct 24, 2018 from issue Qt: Fix for bitcoin-qt becoming unresponsive during shutdown (issue #13217) by LeandroRocha84
  21. ken2812221 approved
  22. ken2812221 commented at 7:28 AM on December 2, 2018: contributor

    tACK 4512ac8bdec9446674e774a83164419caae4b4ed

  23. MarcoFalke commented at 6:08 PM on December 3, 2018: member

    Concept ACK

  24. DrahtBot added the label Needs rebase on Dec 7, 2018
  25. hebasto force-pushed on Dec 7, 2018
  26. hebasto commented at 7:25 PM on December 7, 2018: member

    Rebased.

  27. DrahtBot removed the label Needs rebase on Dec 7, 2018
  28. DrahtBot added the label Needs rebase on Jan 5, 2019
  29. hebasto force-pushed on Jan 6, 2019
  30. hebasto commented at 9:19 AM on January 6, 2019: member

    Rebased.

  31. DrahtBot removed the label Needs rebase on Jan 6, 2019
  32. MarcoFalke closed this on Jan 6, 2019
  33. MarcoFalke reopened this on Jan 6, 2019
  34. DrahtBot added the label Needs rebase on Jan 9, 2019
  35. Remove redundant stopThread() signal 1c0e0a5e38
  36. Remove redundant stopExecutor() signal 24313fbf7e
  37. hebasto force-pushed on Jan 9, 2019
  38. hebasto commented at 11:35 PM on January 9, 2019: member

    Rebased. Again :)

  39. DrahtBot removed the label Needs rebase on Jan 9, 2019
  40. laanwj commented at 1:33 PM on January 17, 2019: member

    utACK 24313fbf7e3d69145bc18c089601ba7aea35d61c

  41. laanwj merged this on Jan 17, 2019
  42. laanwj closed this on Jan 17, 2019
  43. laanwj referenced this in commit 7ee604487f on Jan 17, 2019
  44. hebasto deleted the branch on Jan 17, 2019
  45. jasonbcox referenced this in commit 209532350a on Oct 9, 2020
  46. jasonbcox referenced this in commit b8a022d6d6 on Oct 9, 2020
  47. vijaydasmp referenced this in commit e0a63fd53d on Sep 12, 2021
  48. vijaydasmp referenced this in commit 08c7f50f00 on Sep 12, 2021
  49. vijaydasmp referenced this in commit 1a8ffbe27f on Sep 12, 2021
  50. vijaydasmp referenced this in commit 90c1e2c472 on Sep 12, 2021
  51. vijaydasmp referenced this in commit 833edafcde on Sep 12, 2021
  52. PastaPastaPasta referenced this in commit c88fe10816 on Sep 21, 2021
  53. kwvg referenced this in commit 34bb40a231 on Oct 12, 2021
  54. bitcoin locked this on Dec 16, 2021
Contributors
hebastopromagDrahtBotjonasschnelliSjorsfanquakeken2812221MarcoFalkelaanwj


laanwj

Labels
GUI
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC