Implement experimental NNTP pipelining (#680)#799
Draft
bket wants to merge 4 commits into
Draft
Conversation
Introduces a pipelined request model to ArticleDownloader to mitigate
latency overhead during article retrieval. This allows the client to
dispatch multiple BODY/ARTICLE commands before waiting for the
corresponding multi-line responses.
Changes:
- Options: Added 'ServerX.PipelineDepth' (default: 2). A value of 1
effectively disables pipelining.
- NntpConnection: Decoupled Request() into SendRequest() and
ReadResponseLine() to support asynchronous command/response flow.
- ArticleDownloader:
* Refactored Download() loop to use a std::deque for tracking
in-flight pipeline entries.
* Implemented ReserveNextPipelinedArticle() to speculatively lock
upcoming articles in the FileInfo queue.
* Added RestorePipeline() to handle graceful rollback of 'aiRunning'
status for pending articles on connection failure.
- Error Handling: Integrated 480 (auth) sensing within the pipeline read
loop to trigger mid-stream authentication and request re-send.
- WebUI: Updated configuration and statistics modules to expose and
monitor pipeline depth per server.
WIP Note: This is a rough implementation seeking testing on
high-bandwidth-delay links, across different Usenet providers and
network conditions. More eyes on all parts of the code are welcome,
particularly:
- Connection stability during NNTP authentication (480 responses).
- Thread-safety and state integrity of the ArticleDownloader loop.
- Potential race conditions in ReserveNextPipelinedArticle.
Adjusts the test suite to accommodate the changes introduced by the NNTP pipelining implementation. Changes: - Updated NewsServer instantiation in Options and ServerPool mocks to support the new pipelineDepth parameter. - Refactored ServerFixture and NewsServerValidator test cases to include pipeline depth, ensuring correct argument ordering and maintaining a default depth of 2 for validation tests. - Updated nntp.cmake to include NntpConnection.cpp in the test build to enable verification of the new decoupled request/response logic. WIP Note: These changes ensure the test suite compiles and passes with the experimental pipelining code. Further unit tests specifically targeting pipeline edge cases are still welcome.
Previously, the progress bar and "downloaded size" for an NZB would only update after an article was fully downloaded and validated. When using PipelineDepth > 1, this caused the GUI indicator to appear frozen for long periods. This change moves accounting into the active download loop, providing incremental progress updates. In the event of a download failure or CRC error, the success counters are rolled back accordingly.
Standardize the socket cleanup logic using the project-wide closesocket define. This should fix the NntpConnection test on Windows where close() was called on a socket handle.
Member
|
@bket We also started looking into pipelining implementation and actively working on preparation/prototyping, we most likely would use some of your code, but we have to be accurate about all edge cases and do more testing of course. |
Contributor
Author
|
@luckedea, glad the code can serve as input for the prototype. Please let me know if I can help with the edge cases or testing. Also, feel free to close this PR whenever you are ready, or just give me a heads-up if you would prefer I do it. Happy to see the feature moving forward either way. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Introduces a pipelined request model to ArticleDownloader to mitigate latency overhead during article retrieval. This allows the client to dispatch multiple BODY/ARTICLE commands before waiting for the corresponding multi-line responses. Inspired by https://sabnzbd.org/wiki/advanced/nntp-pipelining (via #680).
Includes a test suite to accommodate changes introduced by the NNTP pipelining implementation.
More details in the commit messages.
Lib changes
No changes.
Testing
Build on OpenBSD. Tested with up to 32 connections and pipeline depths up to 16 on a <10ms latency 1Gb link. Observed stable downloads but need verification on edge cases.
WIP Note: This is a rough implementation seeking testing on high-bandwidth-delay links, across different Usenet providers and network conditions. More eyes on all parts of the code are welcome, particularly:
@dnzbk, @luckedea, your input is much appreciated.