refactor: acquireAsync will dispatch a job, not be dispatched #5327
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8a1331c to
021c10d
Compare
fd9bff2 to
a241e2f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5327 +/- ##
=======================================
Coverage 79.1% 79.1%
=======================================
Files 816 817 +1
Lines 71622 71652 +30
Branches 8237 8234 -3
=======================================
+ Hits 56644 56679 +35
+ Misses 14978 14973 -5
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
021c10d to
7dc9efd
Compare
011e2c8 to
53f1f02
Compare
7dc9efd to
068b71f
Compare
53f1f02 to
16a3282
Compare
068b71f to
d7790ce
16a3282 to
ccb0099
Compare
d7790ce to
46f8af5
Compare
ccb0099 to
a53f0c0
Compare
46f8af5 to
0e4f4d2
Compare
a53f0c0 to
78f4bae
Compare
0e4f4d2 to
06e0540
Compare
78f4bae to
c1382ab
Compare
06e0540 to
4b464d1
Compare
c1382ab to
363cd17
Compare
4b464d1 to
a6799ec
Compare
363cd17 to
0954203
Compare
a6799ec to
0b8c2b3
Compare
0954203 to
adb9df3
Compare
0b8c2b3 to
f2b1a67
Compare
adb9df3 to
262940a
Compare
f2b1a67 to
c1215d0
Compare
262940a to
9ecb783
Compare
c1215d0 to
966204c
Compare
9ecb783 to
e60c51a
Compare
966204c to
c49d81c
Compare
e60c51a to
da31e1c
Compare
c49d81c to
07fa3a6
Compare
99fb808 to
706a4c9
Compare
275a045 to
0a02530
Compare
706a4c9 to
3f71758
Compare
0a02530 to
a7c1d72
Compare
3f71758 to
b92c9f9
Compare
5863223 to
04213be
Compare
04213be to
21ad0cc
Compare
|
Unfortunately, this is not ready for prime time. I ran a simple test over the weekend, running this branch for some time, and running What the data shows is:
Clearly, something is misbehaving, so I introduced a problem somewhere. I may be able to take some time to find out where, but it's not my priority right now, so it may be a while. |
- Improve logging related to ledger acquisition and operating mode changes - Class "CanProcess" to keep track of processing of distinct items
- Delete the copy ctor & operator
7d84119 to
87c61e1
Compare
|
This is ready for review. I accidentally ran the original code in the test mentioned in my previous comment. When I ran it again over the last few days, it had a more de-syncs, but not unreasonably so. About 40 in the last 24 hours. Maybe still enough to be a problem, but not the disaster I observed earlier. |
vvysokikh1
reviewed
Jun 16, 2025
Comment on lines
+98
to
+100
| // TODO: Use structured binding once LLVM 16 is the minimum supported | ||
| // version. See also: https://github.com/llvm/llvm-project/issues/48582 | ||
| // https://github.com/llvm/llvm-project/commit/127bf44385424891eb04cff8e52d3f157fc2cb7c |
There was a problem hiding this comment.
I think we already agreed to support clang16 as a minimum version. Maybe worth switching to that?
Comment on lines
+128
to
+133
| template <class Mutex, class Item> | ||
| std::function<void()> | ||
| insert(Mutex& mtx, std::set<Item>& collection, Item const& item) | ||
| { | ||
| return doInsert<true>(mtx, collection, item); | ||
| } |
There was a problem hiding this comment.
should there be a specialization for other containers types where invalidation does not happen on insert/erase?
| bool isNew = true; | ||
| std::shared_ptr<InboundLedger> inbound; | ||
| { | ||
| ScopedLockType sl(mLock); | ||
| if (stopping_) | ||
| { | ||
| JLOG(j_.debug()) << "Abort(stopping): " << ss.str(); |
There was a problem hiding this comment.
ss.str() is always empty here
| uint256 const& hash, | ||
| std::uint32_t seq, | ||
| InboundLedger::Reason reason) override | ||
| { | ||
| std::unique_lock lock(acquiresMutex_); | ||
| try | ||
| if (auto check = std::make_shared<CanProcess const>( |
There was a problem hiding this comment.
nit: could be auto const check = ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Does a few different things:
CanProcessto use where needed for job deduplication.to_short_string(base_uint)function to help disambiguate some messages about peers and ledgers.InboundLedger::onTimerwithout sending new requests if the complete ledger is available. (Duh.)InboundLedgers::acquireAsyncso that it dispatches a job, and is not called from a dispatched job. This will reduce the number of jobs spun up only to immediately exit.Context of Change
This is the safe subset of changes from #5301, which has problems, plus the item to dispatch from
acquireAsync.Type of Change
API Impact
None