Depends on D3513
Adding client base classes for reactors - bidi reactor
Details
Details
cd services yarn run-backup-service
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 11 ↗ | (On Diff #10653) | Do we access this from multiple threads? Does it need to be atomic? It might be a good idea to investigate reactors in context of threading. It may be the case that we don't need to make this atomic and library takes care of synchronizing the memory, but it is also possible that we need to do that and currently have a lot of subtle bugs. |
| 45–48 ↗ | (On Diff #10653) | Shouldn't we initialize as the first operation? It sounds like a good idea to do that before calling StartWrite |
| 81–83 ↗ | (On Diff #10653) | Do we need this if? terminate checks if this->done |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 11 ↗ | (On Diff #10653) | I agree – we should have a really strong idea of how threading works with reactors |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 11 ↗ | (On Diff #10653) | I agree as well. I wanted to get the grpc's authors perspective on this first if possible: https://linear.app/comm/issue/ENG-914/take-care-of-the-threading-problem-in-the-grpc-async-api If I get no response, I'm going to investigate this on my own. |
| 45–48 ↗ | (On Diff #10653) | No, StartCall has to be called after the first operation like StartWrite, StartRead, etc. https://github.com/grpc/grpc/blob/70fa55155beade94670661f033e91b55f43c9dee/examples/cpp/route_guide/route_guide_callback_client.cc#L108 + all other occurrences of StartCall in this file. |
| 81–83 ↗ | (On Diff #10653) | You're right it is redundant. |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 45–48 ↗ | (On Diff #10653) | Thanks for the clarification. It looks like term initialized confused me as initialization usually happens before any operation - maybe we can find a less confusing name? Alternatively, in the examples you linked, this call happens in the constructor. Is it possible to use the same approach here? |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 45–48 ↗ | (On Diff #10653) | This cannot be done, because prepareRequest is being called here and it is a pure virtual function. What happens when we create an object of a derived class is, first the constructor of the base class is being called. And in the base class, we have only the pure virtual version of this method, therefore we have to call nextWrite (and effectively prepareRequest) explicitly on an object of the derived class. |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 45–48 ↗ | (On Diff #10653) | Ok, that makes sense |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 58–59 ↗ | (On Diff #10747) | Is that the case that when status is non null, done is always true? If that's the case, maybe we don't need done and in isDone we can return this->status != null |
| 72 ↗ | (On Diff #10747) | This can be probably achieved by calling get method on a shared pointer - for me it's less confusing, but up to you: if you prefer the current approach we can keep it |
| services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 58–59 ↗ | (On Diff #10747) | The problem is, this->status cannot be nullptr because it is not a pointer, just a normal object of a class. So I guess it has some initial values(like error code 0). We could keep status as a smart pointer, but I'd keep it consistent across all reactor base classes so if you feel we need this, I can create a task and do it later. |
| 72 ↗ | (On Diff #10747) | I don't mind, no difference for me(even in readability honestly). |