🙉 Enable NACKs behind a feature toggle #127
Conversation
danstiner
changed the base branch from
master
to
danstiner/threadless-transports
danstiner
force-pushed
the
danstiner/re-enable_nacks
branch
from
f938884 to
f694215
Compare
|
I'm a bit torn how far to go with adding tests to this PR, I think to really make the logic testable it should be pulled into more specialized buffer/nack tracking classes. Maybe before merging I should add at least one happy path test of relaying packets, and then as we refactor this we can add more tests. |
src/FtlServer.h
Outdated
| @@ -66,7 +66,7 @@ class FtlServer : public FtlControlConnectionManager | |||
| /** | |||
| * @brief Starts listening for FTL connections on a new thread. | |||
| */ | |||
| void StartAsync(); | |||
| void StartAsync(bool nackLostPackets); | |||
There was a problem hiding this comment.
Any reason nackLostPackets can't be a constructor parameter?
There was a problem hiding this comment.
Yeah it is weird. The issue is FtlServer is constructed before configuration is loaded in Init.
https://github.com/Glimesh/janus-ftl-plugin/blob/master/src/JanusFtl.cpp#L40
We could just move construction of the FtlServer into Init? I think that makes sense, though I'd also then move the control listener construction Init.
Edit: Did that, seems like an improvement
Meant to be a minimal set of changes to bring back the NACK. Further refactoring can come later.
Tested on the homelab without/with feature enabled. Enabling this makes a substantial improvement in all the simulated packet loss scenarios I've done.
Fixes #95