search

ooni / probe

OONI Probe network measurement tool for detecting internet censorship
https://ooni.org/install
BSD 3-Clause "New" or "Revised" License
754 stars 142 forks source link

sessionresolver: refactor and cleanup #2135

Closed bassosimone closed 1 year ago

bassosimone commented 2 years ago

While working on https://github.com/ooni/probe/issues/2121, I noticed that the sessionresolver package needs refactoring and cleanup. I will document in this issue what changes are required while I implement them.

This issue is related to https://github.com/ooni/probe/issues/2121 and https://github.com/ooni/probe/issues/2115.

bassosimone commented 2 years ago

It seems my recent PR, https://github.com/ooni/probe-cli/pull/807, introduced this data race:

==================
WARNING: DATA RACE
Read at 0x00c00010a360 by goroutine 29:
  github.com/lucas-clemente/quic-go/http3.(*client).Close()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/client.go:191 +0x3b
  github.com/lucas-clemente/quic-go/http3.(*RoundTripper).Close()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/roundtrip.go:162 +0x156
  github.com/ooni/probe-cli/v3/internal/netxlite.(*http3Transport).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/http3.go:42 +0x42
  github.com/ooni/probe-cli/v3/internal/netxlite.(*httpTransportErrWrapper).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/http.go:34 +0x42
  github.com/ooni/probe-cli/v3/internal/netxlite.(*httpTransportLogger).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/http.go:76 +0x42
  github.com/ooni/probe-cli/v3/internal/bytecounter.(*httpTransport).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/bytecounter/http.go:38 +0x42
  net/http.(*Client).CloseIdleConnections()
      /opt/hostedtoolcache/go/1.18.2/x64/src/net/http/client.go:949 +0x8d
  github.com/ooni/probe-cli/v3/internal/netxlite.(*DNSOverHTTPSTransport).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/dnsoverhttps.go:113 +0x42
  github.com/ooni/probe-cli/v3/internal/netxlite.(*SerialResolver).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/resolverserial.go:63 +0x47
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.(*Resolver).closeall()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/resolvermaker.go:116 +0x174
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.(*Resolver).closeall-fm()
      <autogenerated>:1 +0x39
  sync.(*Once).doSlow()
      /opt/hostedtoolcache/go/1.18.2/x64/src/sync/once.go:68 +0x101
  sync.(*Once).Do()
      /opt/hostedtoolcache/go/1.18.2/x64/src/sync/once.go:59 +0x46
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.(*Resolver).CloseIdleConnections()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/sessionresolver.go:104 +0x64
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.TestTypicalUsageWithFailure()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/sessionresolver_test.go:77 +0x4a4
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1486 +0x47

Previous write at 0x00c00010a360 by goroutine 32:
  github.com/lucas-clemente/quic-go/http3.(*client).dial()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/client.go:104 +0x15e
  github.com/lucas-clemente/quic-go/http3.(*client).RoundTrip.func1()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/client.go:211 +0xa4
  sync.(*Once).doSlow()
      /opt/hostedtoolcache/go/1.18.2/x64/src/sync/once.go:68 +0x101
  sync.(*Once).Do()
      /opt/hostedtoolcache/go/1.18.2/x64/src/sync/once.go:59 +0x46
  github.com/lucas-clemente/quic-go/http3.(*client).RoundTrip()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/client.go:210 +0x268
  github.com/lucas-clemente/quic-go/http3.(*RoundTripper).RoundTripOpt()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/roundtrip.go:116 +0x928
  github.com/lucas-clemente/quic-go/http3.(*RoundTripper).RoundTrip()
      /home/runner/go/pkg/mod/github.com/lucas-clemente/quic-go@v0.27.0/http3/roundtrip.go:121 +0x38
  github.com/ooni/probe-cli/v3/internal/netxlite.(*http3Transport).RoundTrip()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/http3.go:37 +0x4a
  github.com/ooni/probe-cli/v3/internal/netxlite.(*httpTransportErrWrapper).RoundTrip()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/http.go:26 +0x4e
  github.com/ooni/probe-cli/v3/internal/netxlite.(*httpTransportLogger).RoundTrip()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/http.go:60 +0x2e1
  github.com/ooni/probe-cli/v3/internal/bytecounter.(*httpTransport).RoundTrip()
      /home/runner/work/probe-cli/probe-cli/internal/bytecounter/http.go:50 +0x210
  net/http.send()
      /opt/hostedtoolcache/go/1.18.2/x64/src/net/http/client.go:252 +0x93e
  net/http.(*Client).send()
      /opt/hostedtoolcache/go/1.18.2/x64/src/net/http/client.go:176 +0x157
  net/http.(*Client).do()
      /opt/hostedtoolcache/go/1.18.2/x64/src/net/http/client.go:725 +0x1064
  net/http.(*Client).Do()
      /opt/hostedtoolcache/go/1.18.2/x64/src/net/http/client.go:593 +0x36
  github.com/ooni/probe-cli/v3/internal/netxlite.(*DNSOverHTTPSTransport).RoundTrip()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/dnsoverhttps.go:74 +0x827
  github.com/ooni/probe-cli/v3/internal/netxlite.(*SerialResolver).lookupHostWithoutRetry()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/resolverserial.go:131 +0x23c
  github.com/ooni/probe-cli/v3/internal/netxlite.(*SerialResolver).lookupHostWithRetry()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/resolverserial.go:103 +0x108
  github.com/ooni/probe-cli/v3/internal/netxlite.(*SerialResolver).LookupHost()
      /home/runner/work/probe-cli/probe-cli/internal/netxlite/resolverserial.go:69 +0x8d
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.timeLimitedLookupWithTimeout.func1()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/childresolver.go:39 +0xf7

Goroutine 29 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1486 +0x724
  testing.runTests.func1()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1839 +0x99
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1439 +0x213
  testing.runTests()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1837 +0x7e4
  testing.(*M).Run()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1719 +0xa71
  main.main()
      _testmain.go:181 +0x3a9

Goroutine 32 (running) created at:
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.timeLimitedLookupWithTimeout()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/childresolver.go:37 +0x256
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.timeLimitedLookup()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/childresolver.go:22 +0x91
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.(*Resolver).lookupHost()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/sessionresolver.go:172 +0x293
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.(*Resolver).LookupHost()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/sessionresolver.go:142 +0x4f9
  github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver.TestTypicalUsageWithFailure()
      /home/runner/work/probe-cli/probe-cli/internal/engine/internal/sessionresolver/sessionresolver_test.go:36 +0x170
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.2/x64/src/testing/testing.go:1486 +0x47

==================
--- FAIL: TestTypicalUsageWithFailure (0.00s)
    testing.go:1312: race detected during execution of test
FAIL
coverage: 100.0% of statements
FAIL    github.com/ooni/probe-cli/v3/internal/engine/internal/sessionresolver   0.087s

See https://github.com/ooni/probe-cli/runs/6793211653?check_suite_focus=true. I am going to re-run the tests but obviously we need to understand and fix the data race as part of this PR.