-
Notifications
You must be signed in to change notification settings - Fork 8
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
Allow the SidecarTransport to recreate its underlying transport #440
Conversation
e290c6a
to
b25df51
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #440 +/- ##
==========================================
+ Coverage 67.76% 67.88% +0.12%
==========================================
Files 193 193
Lines 24476 24583 +107
==========================================
+ Hits 16585 16689 +104
- Misses 7891 7894 +3
|
f5d89f6
to
cfe4828
Compare
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.
Too complicated for me ;) but a few comments around testing and comments
} | ||
|
||
impl SidecarTransport { | ||
pub fn reconnect<F>(&mut self, factory: F) |
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.
Could you add utests for all those fonctions?
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.
Added some tests, but I'm not really happy with them as they are really coupled to the BlockingTransport.
Any advice is welcome!
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 that's fine, the functionality itself is coupled to the BlockingTransport, so that's quite natural here.
sidecar/src/service/blocking.rs
Outdated
pub fn is_closed(&self) -> bool { | ||
match self.inner.lock() { | ||
Ok(t) => t.is_closed(), | ||
Err(_) => true, // Well... what can we do? |
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 like that we're not just unwrapping and potentially panicking on the lock acquisitions, but I'm not sure what here is safe? I believe the majority of the time we'd get an error on a lock is if the lock is poisoned, which means we might be in a bad state? is it ok to assume that if self.inner is poisoned that the connection is closed and we should move forward with trying to reconnect?
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.
The scenario where this will happen is when the reconnect callback executes functions on the sidecar instance (only location which calls back to caller from within the lock). And ... during the reconnect the original sidecar instance is always closed (otherwise the reconnect wouldn't happen in the first place), so actually true
is the right value to report there.
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.
This fact should be noted as comment rather than "what can we do" though :-)
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.
Comment updated 😛
fca832a
to
797cd71
Compare
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.
Looks good to me, good for safety in threaded environments, especially avoiding possible risk of cross talk on send() and call().
…se it is broken. This is handled in the Rust code so that sidecar's clients (like the PHP tracer) can use the SidecarTransport in a thread safe way.
eac12cc
to
0a05659
Compare
What does this PR do?
Replace the
SidecarTransport
type alias by a real struct that handles reconnection.This is handled in the Rust code so that sidecar's clients (like the PHP tracer) can use the SidecarTransport in a thread safe way.
Motivation
In the PHP tracer, the reconnection was previously done in the C code, but was not thread safe.
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.