SimpleStream improvements and logging#1123
Open
jameson-dev wants to merge 5 commits intoTrunkRecorder:masterfrom
Open
SimpleStream improvements and logging#1123jameson-dev wants to merge 5 commits intoTrunkRecorder:masterfrom
jameson-dev wants to merge 5 commits intoTrunkRecorder:masterfrom
Conversation
File-scope globals were shared across every instance of the class. If more than one SimpleStream instance were ever loaded, they would silently share the same stream list and TCP I/O service, likely causing data corruption and race conditions.
Copying a struct with a raw pointer wont manage the socket lifetime, and two copies of the same pointer could interact badly. It also needlessly copies two std::string members and a UDP endpoint on every audio packet, which is a hot path. The const auto & form avoids all copies entirely. The start() and stop() loops already used auto& correctly, so this change just introduces consistency
Dropped TCP connections throws boots::system::system_error, which would terminate the TR process. It should now log errors and continue. For UDP, there was no logging for errors to know when packets are dropped
from_string() only accepts literal IP addresses, entering a hostname would throw an error and crash TR with no explanation.
Contributor
|
I really wanted someone to make |
Adds support for a url field in the simplestream config. Can now be defined as udp://hostname:port or tcp://hostname:port instead of separate address, port, and useTCP fields. Added deprecation warning log and updated documentation to reflect changes and deprecation intent
Contributor
Author
Contributor
|
Exactly! |
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 for changes have been added to each commit. Happy for criticism on these changes 😄
Here's a brief breakdown on the changes:
Move streams, my_tcp_io_service, and max_tcp_index from file-scope globals into the Simple_Stream class as private members, preventing shared state between instances
Fix BOOST_FOREACH loops to iterate by const reference rather than copying the struct by value on every audio packet
Wrap TCP send() calls in try/catch to prevent a dropped connection from crashing the entire trunk-recorder process
Log a warning when UDP send_to() fails silently so network errors are visible in logs
Replace ip::address::from_string() with Boost.Asio resolvers for both UDP and TCP connections, allowing hostnames to be used in the config in addition to literal IP addresses
Note: I haven't actually tested these changes at all not having a TR setup to test with at the moment, but I'm rather confident of the changes. I would definitely test these changes first.