services: don't update reflection index mid-stream #2767
Conversation
| * detected. A change is any addition or removal in the set of file descriptors attached to the | ||
| * mutable services or a change in the service names. | ||
| */ | ||
| private void updateIndexIfNecessary() { |
There was a problem hiding this comment.
This is not thread-safe. serverReflectionInfo() may be called from multiple threads. Make it synchronized and put @GuardedBy on serverReflectionIndex?
You'd also need to return serverReflectionIndex from this method, to avoid grabbing the lock again in serverReflectionInfo(). Alternatively you could use a volatile, but meh.
There was a problem hiding this comment.
Lock and @GuardedBy added.
There was a problem hiding this comment.
For posterity: this is actually thread-safe, due to final usage, but it isn't obviously so and it does take some effort to show it is safe. So we're still going to change it to something more obviously correct.
|
Travis is broken right now. Feel free to ignore it. It should be fine in a few more hours. |
2 participants
This addresses #2689.
ServerReflectionIndexis now immutable, and its reference is replaced by a new instance when changes are detected.I also fixed some poor formatting choices present in the earlier code, primarily the tests - I don't think the changes are too invasive, given that much of the code has genuinely been changed here, and the formatting already popped up as an issue on #2657.