From 583d154f9c751eeacdf6d8aa3b4e256a8dfa5aff Mon Sep 17 00:00:00 2001
From: Joshua Humphries <2035234+jhump@users.noreply.github.com>
Date: Tue, 7 Mar 2023 14:25:27 -0500
Subject: [PATCH] Go back to waiting for override descriptor.proto with all
 other deps (#115)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

However, it blocks for the other result while still holding the semaphore!
So this opens up the possibility of deadlock 🤦. A possible remedy is to
release the semaphore before blocking, and re-acquiring when done.
But that seems too complicated/brittle: we already have code that must
release/re-acquire the semaphore, so it seems best to just keep that in
one place. Which means reverting the "clever" change and just waiting
for an override descriptor.proto while waiting for all other imports.
---
 compiler.go | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/compiler.go b/compiler.go
index 4ad0b0b1..d3011862 100644
--- a/compiler.go
+++ b/compiler.go
@@ -435,7 +435,7 @@ func (t *task) asFile(ctx context.Context, name string, r SearchResult) (linker.
 		}
 	}
 
-	var descriptorProtoRes *result
+	var overrideDescriptorProto linker.File
 	if len(imports) > 0 {
 		t.r.setBlockedOn(imports)
 
@@ -457,6 +457,7 @@ func (t *task) asFile(ctx context.Context, name string, r SearchResult) (linker.
 			results[i] = res
 		}
 		deps = make([]linker.File, len(results))
+		var descriptorProtoRes *result
 		if wantsDescriptorProto {
 			descriptorProtoRes = t.e.compile(ctx, descriptorProtoPath)
 		}
@@ -484,7 +485,17 @@ func (t *task) asFile(ctx context.Context, name string, r SearchResult) (linker.
 				return nil, ctx.Err()
 			}
 		}
-
+		if descriptorProtoRes != nil {
+			select {
+			case <-descriptorProtoRes.ready:
+				// descriptor.proto wasn't explicitly imported, so we can ignore a failure
+				if descriptorProtoRes.err == nil {
+					overrideDescriptorProto = descriptorProtoRes.res
+				}
+			case <-ctx.Done():
+				return nil, ctx.Err()
+			}
+		}
 		// all deps resolved
 		t.r.setBlockedOn(nil)
 		// reacquire semaphore so we can proceed
@@ -494,7 +505,7 @@ func (t *task) asFile(ctx context.Context, name string, r SearchResult) (linker.
 		t.released = false
 	}
 
-	return t.link(ctx, parseRes, deps, descriptorProtoRes)
+	return t.link(parseRes, deps, overrideDescriptorProto)
 }
 
 func (e *executor) checkForDependencyCycle(res *result, sequence []string, pos ast.SourcePos, checked map[string]struct{}) error {
@@ -553,23 +564,15 @@ func findImportPos(res parser.Result, dep string) ast.SourcePos {
 	return ast.UnknownPos(res.FileNode().Name())
 }
 
-func (t *task) link(ctx context.Context, parseRes parser.Result, deps linker.Files, descriptorProtoRes *result) (linker.File, error) {
+func (t *task) link(parseRes parser.Result, deps linker.Files, overrideDescriptorProtoRes linker.File) (linker.File, error) {
 	file, err := linker.Link(parseRes, deps, t.e.sym, t.h)
 	if err != nil {
 		return nil, err
 	}
 
 	var interpretOpts []options.InterpreterOption
-	if descriptorProtoRes != nil {
-		select {
-		case <-descriptorProtoRes.ready:
-			// descriptor.proto wasn't explicitly imported, so we can ignore a failure
-			if descriptorProtoRes.err == nil {
-				interpretOpts = []options.InterpreterOption{options.WithOverrideDescriptorProto(descriptorProtoRes.res)}
-			}
-		case <-ctx.Done():
-			return nil, ctx.Err()
-		}
+	if overrideDescriptorProtoRes != nil {
+		interpretOpts = []options.InterpreterOption{options.WithOverrideDescriptorProto(overrideDescriptorProtoRes)}
 	}
 
 	optsIndex, err := options.InterpretOptions(file, t.h, interpretOpts...)