Details

services/lib/src/server-base-reactors/ServerBidiReactorBase.h
100	This API looks strange. I would expect that worker is an internal entity and we interact with pool instead. We submit a task to a pool which schedules it and assigns to a worker when it is available.
101–105	This is really confusing. Why do we have two callbacks provided to the `schedule` method? Looking at D5068 it seems like the second argument is a `callback` which is invoked at the end of the task. Can't we include the logic for callback at the end of the task?

This revision now requires changes to proceed.Sep 9 2022, 5:31 AM

tomek mentioned this in D5070: [services] Rust Integration - c++ - Implement worker in read reactor.Sep 9 2022, 5:32 AM

tomek mentioned this in D5071: [services] Rust Integration - c++ - Implement worker in write reactor.

I understand your concerns. The point here is that I wanted to have a possibility to schedule a task and if this task doesn't fail, then schedule a callback that is called only afterward. If you take a look at the implementations, these tasks/callbacks differ so I think that introducing a separate layer of abstraction makes sense.

Also, in this case, we can easily replace the logic for the thread pooling because it is transparent from places where Worker::schedule is being invoked.

• karol planned changes to this revision.Sep 14 2022, 2:59 AM

rebase

Harbormaster completed remote builds in B12133: Diff 16655.Sep 14 2022, 3:09 AM

Not really qualified yet to give this a proper review.

tomek requested changes to this revision.Sep 15 2022, 7:53 AM

tomek added inline comments.

services/lib/src/server-base-reactors/ServerBidiReactorBase.h
101 ↗	(On Diff #16655)	According to https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0018r3.html it isn't safe to schedule a lambda which captures `this` by reference. If we can guarantee that `this` exists when the task is executed, it should be fine. Please check, if according to the linked document, our approach is safe.

This revision now requires changes to proceed.Sep 15 2022, 7:53 AM

• karol added inline comments.Sep 15 2022, 8:57 AM

services/lib/src/server-base-reactors/ServerBidiReactorBase.h
101 ↗	(On Diff #16655)	This is guaranteed. `this` lives as long as the grpc connection is on. It gets destructed when the grpc connection ends. And basically, these lambdas call the functions that do the grpc operations. Only with these operations, the connection may terminate. So, the only way to delete `this` is to call the functions that are inside of these lambdas. (e.g. `Finish`)