This differential introduces changes that ensure all tasks scheduled in the thread pool that write to memory allocated for reactor instance complete before reactor memory is freed. This differential spans over 3 files but changes are almost identical - reviewing one file is enough.
Details
Details
- Launch blob service without this differential (both locally and on AWS (MUST!!!)).
- Execute yarn run-performance-tests blob several times (remember to change IP address in blob_performance_tests.rs if using AWS). It will crash (malloc error) every 3 - 4 run.
- Introduce this differential. Repeate step 1.
- Execute yarn run-performance-tests blob at least 20 times. Ensure crash disappears. On AWS single run of this test may take up to a couple of minutes depending on you internet connection quality. Do not be afraid if you see messages like:
test blob_performance_test has been running for over 60 seconds. After applying this diff test will finally succeed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/lib/src/server-base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 172 ↗ | (On Diff #17569) | Can we update this to phab.comm.dev? |
Comment Actions
Increment ongoing task counter for OnDone callback. It is needed - otherwise we will have memory leak.
| services/lib/src/server-base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 172 ↗ | (On Diff #17569) | Sure - I will do it. |
Comment Actions
otherwise LGTM
| services/lib/src/server-base-reactors/ServerReadReactorBase.h | ||
|---|---|---|
| 112–119 ↗ | (On Diff #17572) | This should be logically the same, and removes the early return |
| services/lib/src/server-base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 86 ↗ | (On Diff #17572) | actually, this really supposed to go here? I think the RUNNING state means that it's not finalized yet, so we shouldn't mark it as done. I realize this is in the terminate() method, but existing behavior did not mark it as TERMINATE, so I'm not sure what the desired behavior is. |
| services/lib/src/server-base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 86 ↗ | (On Diff #17572) | The reason to put this->finishPoolTask() here is that we must ensure that this->finishPoolTask() is executed at the end of a every callback lambda that is passed to thread pool (second argument of scheduleWithCallback). finishPoolTask() firstly checks if there are no tasks executing on the thread pool and OnDone method has been called (which implies that no more tasks will be scheduled to the thread pool by this reactor). Only if both are true reactor's memory is deleted. Reactor's methods are executed in defined order but tasks they schedule to the pool are not. The objective of this differential is to make sure that memory is deleted in the very last task to be executed in the thread pool. That is why every task calls it at the end, but only one will actually delete memory |
| services/lib/src/server-base-reactors/ServerBidiReactorBase.h | ||
|---|---|---|
| 193–208 ↗ | (On Diff #17602) | Shouldn't we introduce these methods in BaseReactor? |
| services/lib/src/server-base-reactors/ServerBidiReactorBase.h | ||
|---|---|---|
| 193–208 ↗ | (On Diff #17602) | I agree it would be nicer to keep those methods in single file but I am not sure whether BaseReactor is a good place since client-base-reactors also inherit from from BaseReactor but they don't perform any operations on ThreadPool. In my opinion a better option would be to create a class at more specific level of inheritance. Introduce something like ThreadPoolDependentReactor.h that would implement those methods and make server-base-reactors inherit from it. @tomek what do you think? I am willing to extract those methods into a single class/file, just let me know what do you think. |
| services/lib/src/server-base-reactors/ServerBidiReactorBase.h | ||
|---|---|---|
| 193–208 ↗ | (On Diff #17602) | After thinking for a while I am not sure it is going to be so straight-forward. finishPoolTask deletes memory pointed by this. If we implement it in a base class that server-base-reactors would inherit from then this refers to the portion of memory allocated for this particular base class. This would result in a memory leak since memory allocated for the rest of Server{X}ReactorBase would not be freed. Solution would be to add virtual method (called deleteThis for instance) and implement it in each class (implementation would be just delete this;). It is ugly but at least less cumbersome than repeating implementation for each file so I will probably proceed with this solution. I am requesting review prior to making those changes just to make sure reviewers see this comment in advance. It will make review easier once those changes are introduced. |
| services/lib/src/server-base-reactors/ServerBidiReactorBase.h | ||
|---|---|---|
| 193–208 ↗ | (On Diff #17602) | After the discussion we've decided to try an approach where BaseReactor would have a virtual destructor. |