Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Conversation

@igorpeshansky
Copy link
Contributor

No description provided.

Copy link

@rbuskens rbuskens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

<< " body: " << request.body;
if (request.method == "GET" && request.destination.find(kPrefix) == 0) {
std::string id = request.destination.substr(kPrefix.size());
std::lock_guard<std::mutex> lock(agent_.mu_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious -- are there more than 1 read threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the number of server threads is specified in the config (MetadataApiNumThreads). But it's enough to have 1 read thread and 1 write thread to trigger the race condition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. I only ask because read threads should not have to block other read threads. Its only when writes happen or attempts to read when a write is in progress that the mutex actually comes in. Probably a micro-optimization to be deferred for later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true. Will fix this later (I'm not too happy with the direct access to the agent's fields, including the mutex, from a friend class anyway).

Copy link
Contributor

@dhrupadb dhrupadb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@igorpeshansky igorpeshansky merged commit 9098d96 into master Oct 16, 2017
@igorpeshansky igorpeshansky deleted the igorp-race-condition-fix branch October 16, 2017 20:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants