[binder] Fix server-side recv_trailing_metadata#27184
Conversation
TaWeiTu
added
the
release notes: no
TaWeiTu
force-pushed
the
fix-recv-trailing-metadata
branch
from
7ab7547 to
8764e86
Compare
sifmelcara
left a comment
sifmelcara
left a comment
There was a problem hiding this comment.
The transport explainer said "Further examples include the idea that the server logic expects to not complete recv_trailing_metadata until after it actually sends trailing metadata since it would have already found this out by seeing a NULL’ed recv_message. This is considered part of the transport's duties in preserving orders."
However I don't really understand why, would you mind to explain?
| // Trailing metadata marks the end of one-side of the stream. Thus, after | ||
| // receiving trailing metadata from the other-end, we know that there will | ||
| // never be in-coming message data anymore, and all recv_message callbacks | ||
| // (as well as recv_initial_metadata callback, if there's any) registered will | ||
| // never be satisfied. This function cancels all such callbacks gracefully | ||
| // (with GRPC_ERROR_NONE) to avoid being blocked waiting for them. |
There was a problem hiding this comment.
Wrong comment location?
There was a problem hiding this comment.
I think this is the place it should be. Can you elaborate why it's at the wrong location?
| // for them. | ||
| virtual void CancelRecvMessageCallbacksDueToTrailingMetadata( | ||
| StreamIdentifier id) = 0; | ||
| // Remove all entries associated with stream number `id`. |
There was a problem hiding this comment.
Wrong comment location?
| InitialMetadataCallbackType cb = nullptr; | ||
| { | ||
| grpc_core::MutexLock l(&m_); | ||
| auto iter = initial_metadata_cbs_.find(id); | ||
| if (iter != initial_metadata_cbs_.end()) { | ||
| cb = std::move(iter->second); | ||
| initial_metadata_cbs_.erase(iter); | ||
| } | ||
| trailing_metadata_recvd_.insert(id); | ||
| } | ||
| if (cb != nullptr) { | ||
| // The registered callback will never be satisfied. Cancel it. | ||
| cb(absl::CancelledError("")); | ||
| } |
There was a problem hiding this comment.
Probably want a separate member function for readability
I don't quite understand the reason either, but I can give more context on the problem this would cause. From what I've seen, in for example a unary call setting, |
TaWeiTu
force-pushed
the
fix-recv-trailing-metadata
branch
2 times, most recently
from
77a7827 to
1d666e5
Compare
sifmelcara
left a comment
sifmelcara
left a comment
There was a problem hiding this comment.
Although it does not address any real issue, I think it is ok to merge, just make sure your fuzzing target are still happy with these change
The code become more and more complex though, we will need to find time to refactor them if we are not going to rewrite the transport in near future
Hmm, why isn't it addressing any issue? The (new) tests in #27179 would fail without this due to the reason I outlined above.
Agree. |
Ah, ok! I missed that |
TaWeiTu
force-pushed
the
fix-recv-trailing-metadata
branch
4 times, most recently
from
22916bd to
6bfda58
Compare
According to the [transport explainer](https://grpc.github.io/grpc/core/md_doc_core_transport_explainer.html), the server-side `recv_trailing_metadata` should not be completed before sending trailing metadata to the client.
TaWeiTu
force-pushed
the
fix-recv-trailing-metadata
branch
from
6bfda58 to
f46f03b
Compare
According to the [transport explainer](https://grpc.github.io/grpc/core/md_doc_core_transport_explainer.html), the server-side `recv_trailing_metadata` should not be completed before sending trailing metadata to the client.
According to the [transport explainer](https://grpc.github.io/grpc/core/md_doc_core_transport_explainer.html), the server-side `recv_trailing_metadata` should not be completed before sending trailing metadata to the client.
2 participants
According to the transport explainer, the server-side
recv_trailing_metadatashould not be completed before sending trailing metadata to the client.Depends on #27182.