👋 @ejona86 any update on this PR? is this still on track for v61 release?

Sorry I went silent on this. I had been hoping to sneak it into 1.60 but then issues with the release cropped up that ended up taking a lot of my time. (I actually didn't see your comment two days ago either...)

We discussed in our API review and the general verdict was "make it a filter like on server-side." Having it just be a hook caused a whole slew of naming/identity/purpose crises that are all solved by mirroring the existing server-side API. Before my time evaporated I was going to actually make the necessary plumbing change...

The listener could receive and return Attributes, just like is done on server-side. In OkHttpClientTransport, BinderTransport, InProcessTransport, CronetClientTransport look to be pretty easy to support (some places attributes would no longer be final; that's okay). NettyClientHandler could pass the Attributes through ClientTransportLifecycleManager.nofiyReady() and update the attributes on return.

But I think that would be inherently racy, in a way not present on server-side. transportReady() is when the client can start sending RPCs on the transport, so we'd need to synchronize the attribute update. On server-side, the server's transport thread can update the attributes before processing any RPCs from the connection.

So instead of adding Attributes to transportReady(), let's add a new method to ManagedClientTransport.Listener just to do this filtering. It can be called just before transportReady(). With a default implementation that just echos back the same attributes, I hope there's not even many tests that have to be updated.

(FYI, the branch cut for 1.61 is a week earlier than normal, or maybe a bit more. So it is coming up soon. I'd be fine backporting this to the release, even though it is technically a feature, if we can do so at least a week before the release. If there's any bugs, seem they should only impact you and not destabilize others.)

Sorry I went silent on this. I had been hoping to sneak it into 1.60 but then issues with the release cropped up that ended up taking a lot of my time. (I actually didn't see your comment two days ago either...)

We discussed in our API review and the general verdict was "make it a filter like on server-side." Having it just be a hook caused a whole slew of naming/identity/purpose crises that are all solved by mirroring the existing server-side API. Before my time evaporated I was going to actually make the necessary plumbing change...

The listener could receive and return Attributes, just like is done on server-side. In OkHttpClientTransport, BinderTransport, InProcessTransport, CronetClientTransport look to be pretty easy to support (some places attributes would no longer be final; that's okay). NettyClientHandler could pass the Attributes through ClientTransportLifecycleManager.nofiyReady() and update the attributes on return.

But I think that would be inherently racy, in a way not present on server-side. transportReady() is when the client can start sending RPCs on the transport, so we'd need to synchronize the attribute update. On server-side, the server's transport thread can update the attributes before processing any RPCs from the connection.

So instead of adding Attributes to transportReady(), let's add a new method to ManagedClientTransport.Listener just to do this filtering. It can be called just before transportReady(). With a default implementation that just echos back the same attributes, I hope there's not even many tests that have to be updated.

Thanks for getting back to me! I addressed these suggestions, ptal and let me know if i understood you correctly. As for backporting this to 1.61, I think we're fine with waiting for 1.62 if you folks are at all worried about the risk. Thanks again!

@joybestourous The builds failed because of CronetClientTransportTest. That test uses a mock for clientTransportListener which by default returns null for all of the methods. You will need to put a when clause into setUp() so that clientTransportListener.filterTransport(attr) just returns its argument.

For example:

when(clientTransportListener.filterTransport(any()).thenAnswer(i -> i.getArguments()[0])

@joybestourous The builds failed because of CronetClientTransportTest. That test uses a mock for clientTransportListener which by default returns null for all of the methods. You will need to put a when clause into setUp() so that clientTransportListener.filterTransport(attr) just returns its argument.

For example:

when(clientTransportListener.filterTransport(any()).thenAnswer(i -> i.getArguments()[0])

Thanks for bringing this to completion 🙏 sorry I missed these errors my android setup was not working properly.

#10805 is suspicious, and shouldn't be necessary. I feel like there's no harm happening at present with making the field volatile, but the fact TSAN is finding issues makes clear something is happening that we don't expect. So we aren't going to backport this into the 1.61 release. We'll probably try to remove the volatile before 1.62, but even if we don't, at that point it will have seen enough testing that we are then more confident it won't cause an issue.

#10805 is suspicious, and shouldn't be necessary. I feel like there's no harm happening at present with making the field volatile, but the fact TSAN is finding issues makes clear something is happening that we don't expect. So we aren't going to backport this into the 1.61 release. We'll probably try to remove the volatile before 1.62, but even if we don't, at that point it will have seen enough testing that we are then more confident it won't cause an issue.

Oh hm that's super surprising / suspicious. Makes complete sense to put this in 1.62. Thanks for the heads up!