[BUGFIX] Fix race condition in kvstore.pushpull #17007
Conversation
| server->Response(req); | ||
| } | ||
| update_buf->request.clear(); | ||
| if (has_multi_precision_copy(type)) CopyFromTo(stored, store_[key]); |
There was a problem hiding this comment.
Can we extract these two lines out of the if and else block since it is called in both conditions.
There was a problem hiding this comment.
The order is different. In this branch, it is done after Response with ACK so that we send back message as early as possible. We then push to engine to perform copy.
For the previous case, we need to response with real data instead of ACK, we actually need to perform copy first.
| } | ||
| if (has_pull) { | ||
| // if there is a pull request, perform WaitToRead() once before DefaultStorageResponse | ||
| if (has_multi_precision_copy(type)) CopyFromTo(stored, store_[key]); |
There was a problem hiding this comment.
nit: it's better to move the CopyFromTo() into a different line and add braces around the if block.
There was a problem hiding this comment.
In general we follow google's cpp style guide. For this case we inline the short statement to improve readability. https://google.github.io/styleguide/cppguide.html#Conditionals
* add back gluon test * fix typo * change back gpu ctx * also handle the case there some are pull and some are pushpull * fix typo
Description
There was a race condition in the previous implementation for the
pushpullAPI. We have to addstored.WaitToRead()before sending it back to workers.@anandj91
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments