netty,okhttp,testing: always set TRANSPORT_ATTR_REMOTE_ADDR#4217
netty,okhttp,testing: always set TRANSPORT_ATTR_REMOTE_ADDR#4217zpencer merged 8 commits intogrpc:masterfrom
Conversation
Always set the remote address, no reason why this should be a TLS-only feature. This is needed for channelz, and is especially useful in unit tests where we are using plaintext.
|
I think I can loadbalance this PR over to @dapengzhang0 |
zpencer
requested review from
dapengzhang0
and removed request for
carl-mastrangelo
| } | ||
|
|
||
| public Attributes getAttributes() { | ||
| synchronized (lock) { |
There was a problem hiding this comment.
lock is not needed if attributes is volatile
There was a problem hiding this comment.
This lock is required to make the @GuardedBy around the transport happy.
There was a problem hiding this comment.
Then maybe you can cache the attribute in TransportState constructor.
There was a problem hiding this comment.
At the time TransportState is constructed, start may not have been called yet. Copying the attributes to a volatile at start or stream allocated will work, but adds more things for us to keep track of. I opted to suppress the warning, PTAL.
There was a problem hiding this comment.
TransportState constructor is invoked by OkHttpClientStream constructor which in turn is invoked by OkHttpClientTransport.newStream(). The newStream() method on real transport won't be called until the real transport is ready (After onTransportReady() called).
// InternalSubchannel.java
/**
* The transport for new outgoing requests. 'lock' must be held when assigning to it. Non-null
* only in READY state.
*/
@Nullable
private volatile ManagedClientTransport activeTransport;| private final int maxMessageSize; | ||
| private int connectionUnacknowledgedBytesRead; | ||
| private ClientFrameHandler clientFrameHandler; | ||
| private Attributes attributes; |
There was a problem hiding this comment.
- maybe volatile
- maybe init with EMPTY
There was a problem hiding this comment.
You're right, this needs to be volatile and set while holding the lock.
carl-mastrangelo
left a comment
carl-mastrangelo
left a comment
There was a problem hiding this comment.
I would need to be further convinced this change is correct. How does this not interfere with the SSL or ALTS negotiators? They have to to call the method too.
|
|
| sock.setTcpNoDelay(true); | ||
| source = Okio.buffer(Okio.source(sock)); | ||
| sink = Okio.buffer(Okio.sink(sock)); | ||
| attrs = Attributes |
There was a problem hiding this comment.
You can set attributes field directly here. Otherwise LGTM.
| @SuppressWarnings("GuardedBy") | ||
| public Attributes getAttributes() { | ||
| return transport.getAttributes(); | ||
| } |
There was a problem hiding this comment.
You can get the attributes and save it as a field at TransportState constructor, the transport is guaranteed in READY state when the constructor is called.
There was a problem hiding this comment.
Done. This required updating some unit tests to ensure streams are not started until the transport is ready.
- copy the attr at construction time - update unit tests to wait until transport is ready before creating streams
|
@carl-mastrangelo do you have any remaining concerns with this PR? |
|
@carl-mastrangelo ping |
| verify(listener, timeout(100)).transportReady(); | ||
| } | ||
|
|
||
|
|
3 participants
Always set the remote address, no reason why this should be a TLS-only
feature. This is needed for channelz, and is especially useful in unit
tests where we are using plaintext.