Skip to content

Commit

Permalink
hotfix(sessionresolver): prevent data race inside http3 (#809)
Browse files Browse the repository at this point in the history
  • Loading branch information
bassosimone committed Jun 8, 2022
1 parent fe29b43 commit f1b8071
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 41 deletions.
25 changes: 7 additions & 18 deletions internal/engine/internal/sessionresolver/childresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,27 +22,16 @@ func timeLimitedLookup(ctx context.Context, re model.Resolver, hostname string)
return timeLimitedLookupWithTimeout(ctx, re, hostname, defaultTimeLimitedLookupTimeout)
}

// timeLimitedLookupResult is the result of a timeLimitedLookup
type timeLimitedLookupResult struct {
addrs []string
err error
}

// timeLimitedLookupWithTimeout is like timeLimitedLookup but with explicit timeout.
func timeLimitedLookupWithTimeout(ctx context.Context, re model.Resolver,
hostname string, timeout time.Duration) ([]string, error) {
outch := make(chan *timeLimitedLookupResult, 1) // buffer
// In https://github.com/ooni/probe-cli/pull/807, I modified this code to
// run in a background goroutine and this resulted in a data race, see
// https://github.com/ooni/probe/issues/2135#issuecomment-1149840579. While
// I could not reproduce the data race in a simple way, the race itself
// seems to happen inside the http3 package. For now, I am going to revert
// the change causing the race and I'll investigate later.
ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
go func() {
out := &timeLimitedLookupResult{}
out.addrs, out.err = re.LookupHost(ctx, hostname)
outch <- out
}()
select {
case <-ctx.Done():
return nil, ctx.Err()
case out := <-outch:
return out.addrs, out.err
}
return re.LookupHost(ctx, hostname)
}
23 changes: 0 additions & 23 deletions internal/engine/internal/sessionresolver/childresolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"errors"
"io"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/ooni/probe-cli/v3/internal/model/mocks"
Expand Down Expand Up @@ -43,25 +42,3 @@ func TestTimeLimitedLookupFailure(t *testing.T) {
t.Fatal("expected nil here")
}
}

func TestTimeLimitedLookupWillTimeout(t *testing.T) {
done := make(chan bool)
block := make(chan bool)
re := &mocks.Resolver{
MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
defer close(done)
<-block
return nil, io.EOF
},
}
ctx := context.Background()
out, err := timeLimitedLookupWithTimeout(ctx, re, "dns.google", 10*time.Millisecond)
if !errors.Is(err, context.DeadlineExceeded) {
t.Fatal("not the error we expected", err)
}
if out != nil {
t.Fatal("expected nil here")
}
close(block)
<-done
}

0 comments on commit f1b8071

Please sign in to comment.