[CommCoreModule] Set `readyState` to `SocketStatus::CLOSED` on `ClientGetReadReactor::OnDone`
ClosedPublic
Actions

Authored by atul on Feb 7 2022, 5:10 AM.

Details

Reviewers

Commits

Summary

Set readyState to 3: CLOSED once the Get() stream is closed.

Test Plan

Logged readyState and it was as expected

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

atul requested review of this revision.Feb 7 2022, 5:17 AM

• karol added inline comments.

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp
59 ↗	(On Diff #9334)	Should we provide a name `status`? Should we handle different values of `status`? Is there a possibility of encountering an error? Should we throw in such a case?
native/cpp/CommonCpp/grpc/ClientGetReadReactor.h
31 ↗	(On Diff #9334)	How about going for consistency?

This revision is now accepted and ready to land.Feb 7 2022, 7:20 AM

This revision now requires review to proceed.Feb 7 2022, 7:20 AM

native/cpp/CommonCpp/grpc/ClientGetReadReactor.h
31 ↗	(On Diff #9334)	Thanks for catching that, will update this diff with that change

address feedback

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp
59 ↗	(On Diff #9334)	Should we handle different values of status? Is there a possibility of encountering an error? Should we throw in such a case? I don't think so? `OnDone` is called by the gRPC threads when the `Get` stream is closed . `status` gives us an idea of why it may have closed, but I think we only need to worry about setting `readyState` correctly. I definitely don't think we want to throw, we should be able to handle things gracefully with just `readyState`