Implement google-c2p resolver. #25215
Conversation
markdroth
added
the
release notes: no
markdroth
force-pushed
the
c2p_resolver
branch
from
cba6cef to
80ede59
Compare
| void* arg, grpc_error* error) { | ||
| auto* self = static_cast<MetadataQuery*>(arg); | ||
| self->MaybeCallOnDone(error); | ||
| grpc_http_response_destroy(&self->response_); |
There was a problem hiding this comment.
This looks like a potentially use-after-free since we are destroying response_ here, but in MaybeCallDone we just potentially passed it to a closure scheduled on the work serializer.
Any reason to not destroy response_ in the MetadataQuery's dtor?
There was a problem hiding this comment.
Good catch! And yes, that's exactly the right fix. Done.
| if (on_done_called_.CompareExchangeStrong( | ||
| &expected, true, MemoryOrder::RELAXED, MemoryOrder::RELAXED)) { | ||
| // Hop back into WorkSerializer. | ||
| Ref().release(); // Ref held by callback. |
There was a problem hiding this comment.
If I'm correct, we need to ref error here, since it's only current ref is the OnHttpRequestDone closure, which may finish before the work serializer closure runs?
There was a problem hiding this comment.
Actually, I should have reffed it in the caller, because this function should take ownership of it, as per https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md#ownership-rules.
Fixed.
| }, | ||
| DEBUG_LOCATION); | ||
| } else { | ||
| GRPC_ERROR_UNREF(error); |
There was a problem hiding this comment.
related to above comment, isn't error's only ref held by the OnHttpRequestDone closure, so unreff'ing here is too much?
There was a problem hiding this comment.
I think the above fix covers this as well.
|
|
||
| void GoogleCloud2ProdResolver::ZoneQueryDone(std::string zone) { | ||
| zone_query_.reset(); | ||
| zone_ = std::move(zone); |
There was a problem hiding this comment.
since in case of error we will be filling in zone_ with the empty string, does TD understand an empty string zone?
There was a problem hiding this comment.
Good point. I've changed the code to not set the locality at all if zone_ is the empty string.
| void* arg, grpc_error* error) { | ||
| auto* self = static_cast<MetadataQuery*>(arg); | ||
| self->MaybeCallOnDone(error); | ||
| grpc_http_response_destroy(&self->response_); |
There was a problem hiding this comment.
Good catch! And yes, that's exactly the right fix. Done.
| if (on_done_called_.CompareExchangeStrong( | ||
| &expected, true, MemoryOrder::RELAXED, MemoryOrder::RELAXED)) { | ||
| // Hop back into WorkSerializer. | ||
| Ref().release(); // Ref held by callback. |
There was a problem hiding this comment.
Actually, I should have reffed it in the caller, because this function should take ownership of it, as per https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md#ownership-rules.
Fixed.
| }, | ||
| DEBUG_LOCATION); | ||
| } else { | ||
| GRPC_ERROR_UNREF(error); |
There was a problem hiding this comment.
I think the above fix covers this as well.
|
|
||
| void GoogleCloud2ProdResolver::ZoneQueryDone(std::string zone) { | ||
| zone_query_.reset(); | ||
| zone_ = std::move(zone); |
There was a problem hiding this comment.
Good point. I've changed the code to not set the locality at all if zone_ is the empty string.
|
The test failures both look like infrastructure issues. |
As part of this, I've changed the
ResolverAPI such that the base class no longer takes ownership of theWorkSerializerorResultHandler. That will require internal changes, so I'll put together a cherry-pick for that once we're happy with this PR.I don't really have a good way to test the C2P resolver, but I'm hoping that you can put together an interop test that will cover it.