Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition #34 #42

Closed
wants to merge 3 commits into from
Closed

Conversation

kevioke
Copy link

@kevioke kevioke commented Apr 13, 2016

Fix race condition #34

@kevioke
Copy link
Author

kevioke commented Apr 20, 2016

@klizhentas can you help me with this PR?

@@ -138,6 +148,10 @@ func (m *RTMetrics) Append(other *RTMetrics) error {
return err
}

m.statusCodesLock.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a potential deadlock here:

a.Append(b)

and

b.Append(a)

will deadlock.

To mitigate the issue, I would suggest to do the following:

  1. implement Export function that uses RLock and returns the copy of the map with status codes
  2. in append, do this:
copied := other.Export()
m.Lock()
defer m.Unlock()
do the merge....
return 

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I'll update this.

@klizhentas
Copy link
Contributor

done, sorry for the delay. two notes:

  1. about possible deadlock (see above)
  2. general thought - why not guard histogram as well and not just status codes?

Thanks

@kevioke
Copy link
Author

kevioke commented Apr 29, 2016

Hi @klizhentas, sorry for the delay I was busy with some other work. I've update the PR for the two points you've mentioned.

Let me know if I missed anything. Happy to make changes.

@ldez
Copy link
Member

ldez commented Nov 10, 2017

any news on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants