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: member

    <!--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. jonasschnelli commented at 11:59 AM on September 25, 2018: contributor

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

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

    <!--32850dd3fdea838b4049e64f46995ea2-->

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

    @Sjors Could you possibly to review this PR?

  11. 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.

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

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

  13. 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.

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

    Added similar cleanup for RPCExecutor. Please re-review.

  18. fanquake removed this from the "Mergeable" column in a project
  19. ken2812221 approved
  20. ken2812221 commented at 7:28 AM on December 2, 2018: contributor

    tACK 4512ac8bdec9446674e774a83164419caae4b4ed

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

    Concept ACK

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

    Rebased.

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

    Rebased.

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

    Rebased. Again :)

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

    utACK 24313fbf7e3d69145bc18c089601ba7aea35d61c

  39. laanwj merged this on Jan 17, 2019
  40. laanwj closed this on Jan 17, 2019
  41. laanwj referenced this in commit 7ee604487f on Jan 17, 2019
  42. hebasto deleted the branch on Jan 17, 2019
  43. jasonbcox referenced this in commit 209532350a on Oct 9, 2020
  44. jasonbcox referenced this in commit b8a022d6d6 on Oct 9, 2020
  45. vijaydasmp referenced this in commit e0a63fd53d on Sep 12, 2021
  46. vijaydasmp referenced this in commit 08c7f50f00 on Sep 12, 2021
  47. vijaydasmp referenced this in commit 1a8ffbe27f on Sep 12, 2021
  48. vijaydasmp referenced this in commit 90c1e2c472 on Sep 12, 2021
  49. vijaydasmp referenced this in commit 833edafcde on Sep 12, 2021
  50. PastaPastaPasta referenced this in commit c88fe10816 on Sep 21, 2021
  51. kittywhiskers referenced this in commit 34bb40a231 on Oct 12, 2021
  52. DrahtBot 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-04-21 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me