Implement SVS v3 bootstrap-time state vectors and async pub/sub support#36
Implement SVS v3 bootstrap-time state vectors and async pub/sub support#36matianxing1992 wants to merge 10 commits into
Conversation
| " elapsed_ms=" << (result.workerUs / 1000)); | ||
|
|
||
| m_face.getIoContext().post([this, alive, result = std::move(result)] () mutable { | ||
| boost::asio::post(m_face.getIoContext(), [this, alive, result = std::move(result)] () mutable { |
There was a problem hiding this comment.
this commit should be folded with the commit that first introduced this code, it shouldn't be a separate commit. Same with the other asio::post changes.
| // DummyClientFace stores sent Interests in a std::vector. On macOS with | ||
| // libc++ container annotations and ASan, parsing the vector-owned Interest | ||
| // directly can trip container-overflow checks in ndn-cxx Block internals. | ||
| // Re-decode the wire into a local Interest before inspecting parameters. |
There was a problem hiding this comment.
I don't understand this and it probably doesn't make sense. Is this AI generated?
Please explain because this may be papering over an actual bug somewhere...
There was a problem hiding this comment.
Thanks for pointing this out. That workaround was added while I was debugging the macOS ASan failure, but I agree that the explanation was not well justified and could mask a real issue.
I removed this workaround in the latest revision. The test now parses the Interest parameters directly again, and the branch has been rebased/cleaned up so this change is no longer part of the PR.
If ASan still reports a problem after the cleanup, I will investigate the actual failing path instead of adding this kind of defensive re-encoding.
There was a problem hiding this comment.
Yes, this was AI-assisted. I saw a large number of macOS/ASan failures and I did not fully understand the root cause at that moment, so I used AI to help debug it.
There was a problem hiding this comment.
To be honest, when I initially completed this large set of patches, the codebase was in a much worse state than it is now. I used AI to help reorganize and clean up portions of the code; otherwise, it would have been even more difficult to review. I apologize for the additional time and effort this has required from you during the review process.
| inline Name::Component | ||
| makeSeqNoComponent(SeqNo seqNo) | ||
| { | ||
| return Name::Component::fromSequenceNumber(seqNo); | ||
| } | ||
|
|
||
| inline SeqNo | ||
| readSeqNoComponent(const Name::Component& component) | ||
| { | ||
| return component.toSequenceNumber(); | ||
| } |
There was a problem hiding this comment.
why bother with these wrappers at all? just call the function directly
There was a problem hiding this comment.
Thank you very much for your suggestions! I have to admit that I was mostly focused on getting the functionality working, and I did not pay nearly enough attention to code quality and coding standards.
matianxing1992
force-pushed
the
master
branch
2 times, most recently
from
2d90595 to
0521665
Compare
| } | ||
|
|
||
| time::system_clock::time_point | ||
| KeyChainSigner::getFreshInterestTimestamp() const |
There was a problem hiding this comment.
why can you not use the InterestSigner class from ndn-cxx? If thread safety is a concern, then you have a much bigger problem because KeyChain doesn't guarantee thread safety either.
Delegate signed Interest nonce/timestamp generation to ndn-cxx's InterestSigner instead of maintaining duplicate timestamp state in SVS.
Add regression coverage for bootstrap-time state vectors, async publish, piggyback delivery, and related pub/sub behavior in the existing unit test suites.
Snapshot pending publication keys before issuing fetches so callbacks and cleanup paths can update m_fetchMap without invalidating the active iteration. This fixes NDNSF dummy-face SVS smoke crashes observed during post-commit validation.
2 participants
This PR adds bootstrap-time state vectors and several async/pubsub delivery improvements to ndn-svs.
Key changes:
fetchAllpending-fetch mutationThe final state of this branch has been thoroughly tested and verified through extensive Mini-NDN experiments. However, because Codex was used to help reorganize and structure the commit history, I cannot guarantee that each intermediate commit represents a stable or fully working state. Only the final commit has been comprehensively validated.