-
Notifications
You must be signed in to change notification settings - Fork 23
RSDK-10435: Dial direct #430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 21 commits
a56574f
d86eac7
4d5f8aa
6eb0fdd
d247cc8
4b7e4c2
c41325b
c549c07
a21c424
41e6960
d0c3f21
1f41cad
a53494a
bbc0adf
a3add02
1ae3732
c3ff877
7d8175b
26c2225
0cd8723
d3cdb58
c0e2197
beab7d2
89104c9
8d68fd2
29cfd35
fde961c
44d8601
c7d8b4c
afa6c70
8ba5efa
95dc5f8
f86011f
4a82a6d
8e99746
a8b858d
2e2b2ec
67aec53
5a3d47c
350f0ff
fa1f598
cb7ec67
ed6bfed
2c7a942
5f9d3b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -254,13 +254,12 @@ ModuleService::ModuleService(int argc, | |
} | ||
|
||
ModuleService::~ModuleService() { | ||
// TODO(RSDK-5509): Run registered cleanup functions here. | ||
VIAM_SDK_LOG(info) << "Shutting down gracefully."; | ||
VIAM_SDK_LOG(info) << "Shutting down gracefully.\n"; | ||
server_->shutdown(); | ||
|
||
if (parent_) { | ||
try { | ||
parent_->close(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was causing a double call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think yes in theory but it's probably pretty tricky, see here for discussion (old conan docs) https://docs.conan.io/1/howtos/sanitizers.html |
||
parent_.reset(); | ||
} catch (const std::exception& exc) { | ||
VIAM_SDK_LOG(error) << exc.what(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised to see this as a global, but maybe I'm misunderstanding. But if you had one channel that was doing direct dial and another that wasn't, wouldn't this by trying to add this metadata to the contexts for both channels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, although I'm not sure how common/feasible such a situation would be in practice, or if we should error out if we detect that kind of thing? cc @stuqdog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this is a good question. I'm not sure how common such a situation is in practice currently, but I do think it's certainly feasible and I could imagine a case where, e.g., someone is connecting to two machines at once and would prefer to use direct gRPC but is required to use webRTC for some of its added functionality on one of the machines only.
For that matter, even if both channels were dialing direct, if they were dialing to different machines then the token would only be valid for one of them! So yeah, probably tokens should either not be global or (if that's not feasible) should be keyed to a particular channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm ok so this is probably something we should handle better. i'll try to look into the architecture and see how to make this work probably on a per channel basis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acmorrow after much wailing and gnashing of teeth we figured out a way to do this without a global, thereby ballooning the surface area of this PR considerably.