Go back to waiting for override descriptor.proto with all other deps#115
Merged
Go back to waiting for override descriptor.proto with all other deps#115
Conversation
pkwarren
approved these changes
Mar 7, 2023
Member
pkwarren
left a comment
There was a problem hiding this comment.
Nice catch. Should've seen this earlier.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #109, trying to be clever, I moved the place where we block for an override descriptor.proto to "just in time", before options are interpreted. That seemed like a potential latency improvement to allow compilation of the override descriptor.proto to overlap with the linking of the file in question, if the file in question didn't actually directly depend on descriptor.proto.
But this decision caused a deadlock that is not reproducible in the tests in this repo: https://github.com/bufbuild/buf/actions/runs/4357071661/jobs/7615922964
So I've reverted that one change, which was the only change even slightly related to concurrency. It turns out that this deadlocks because we now have a situation where the file might block for the result of another file, but while holding on to the semaphore! In order to eliminate the deadlock, an alternative would have been to release the semaphore before blocking for the override file (and then re-acquire it again after the override file result is available).
Oops.