kernel: Remove dependency on CScheduler #28960
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
TheCharlatan
force-pushed
the
noGlobalSignals
branch
3 times, most recently
from
04f3369
to
ca2d514
Compare
|
Concept ACK |
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
ca2d514
to
28f498d
Compare
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
28f498d
to
f86c3e8
Compare
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
f86c3e8
to
5d7605a
Compare
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | (grep -v "$3" || cat;) | xargs sed -i "s/$1/$2/g"; }
s 'SingleThreadedSchedulerClient' 'SerialTaskRunner' ''
s 'SinglethreadedSchedulerClient' 'SerialTaskRunner' ''
s 'm_schedulerClient' 'm_task_runner' ''
s 'AddToProcessQueue' 'insert' ''
s 'EmptyQueue' 'flush' ''
s 'CallbacksPending' 'size' 'validation'
sed -i '109s/CallbacksPending/size/' src/validationinterface.cpp
-END VERIFY SCRIPT-
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s 'CMainSignals' 'ValidationSignals'
s 'MainSignalsImpl' 'ValidationSignalsImpl'
-END VERIFY SCRIPT-
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
c02fd03
to
4e270f3
Compare
|
Thank you for the review @vasild, Updated c02fd03 -> 4e270f3 (noGlobalSignals_15 -> noGlobalSignals_16, compare)
|
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
4e270f3
to
7593105
Compare
|
Updated 4e270f3 -> 7593105 (noGlobalSignals_16 -> noGlobalSignals_17, compare)
|
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
7593105
to
316c7c8
Compare
|
Updated 7593105 -> 316c7c8 (noGlobalSignals_17 -> noGlobalSignals_18, compare) |
| "wallet_tests/importwallet_rescan" # validation.cpp: if (signals.CallbacksPending() > 10) | ||
| "wallet_tests/ListCoins" # validation.cpp: if (signals.CallbacksPending() > 10) | ||
| "wallet_tests/scan_for_wallet_transactions" # validation.cpp: if (signals.CallbacksPending() > 10) | ||
| "wallet_tests/wallet_disableprivkeys" # validation.cpp: if (signals.CallbacksPending() > 10) |
There was a problem hiding this comment.
Would be a nice follow-up to remove the non-deterministic scheduler from all tests, unless they are explicitly testing the scheduler.
cc @aureleoules , who was asking for a solution to this for the corecheck infra
There was a problem hiding this comment.
ACK 316c7c8 🔁
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 316c7c845036cbffa22b9e44f31dca8573ffb639 🔁
1vioJirfjv5T1d+KJlrtR+Bw8ftU7msPN3OGieSZu7FZDuBMu3u642FgGntJXux25f1uaFTvhCgjKjKJDYshCw==
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design. Removing CScheduler also allows removing the thread and exception modules from the kernel library.
TheCharlatan
force-pushed
the
noGlobalSignals
branch
from
316c7c8
to
d5228ef
Compare
|
Thank you for the comments, Updated 316c7c8 -> d5228ef (noGlobalSignals_18 -> noGlobalSignals_19, compare) |
There was a problem hiding this comment.
ACK d5228ef
I would be happy to re-review if this is changed to use automatic memory management everywhere (#28960 (comment) and #28960 (comment)). Or at least add a comment next to the raw pointers/references like "this points to NodeContext::validation_signals and should not be used after that is destroyed".
|
re-ACK d5228ef 🌄 Show signatureSignature: |
|
Added to 28.0, since it looks rfm, but is probably waiting on the branch-off? |
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing
CScheduleralso allows removing the thread and exception modules from the kernel library.To make the
CMainSignalsclass easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.Renames
CMainSignalstoValidationSignals, which more accurately describes its purpose and scope.Also make the
ValidationSignalsin theChainstateManagerand CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.This PR is part of the libbitcoinkernel project. It improves the kernel API and removes two modules from the kernel library.