-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* 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
pushpull
API. 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