cleanup(engine): do not expose LookupLocationContext (ooni#1537)

Potentially, this code would cause a behavioral change in that once the
probe location has been found, it would not change again and it might be
a problem for very-long-running sessions.

However:

1. the Android codebase does not keep a reference to a session for a
very long time and anyway the longest-running sessions are those used
for running experiments;

2. the correct behavior would be for MaybeLookupLocationContext to cache
the results only for a limited amount of time.

Because of all these considerations, it actually makes sense to say that
replacing LookupLocationContext with MaybeLookupLocationContext and
engine.Session accessors is the ~same.

The net benefit for us is that we can further reduce the surface of
interaction between clients and the engine code. A simpler API surface
is also simpler to document.

Part of ooni/probe#2700

internal/engine/session.go

@@ -654,9 +654,9 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
 	return nil
 }
 
-// LookupLocationContext performs a location lookup. If you want memoisation
+// doLookupLocationContext performs a location lookup. If you want memoisation
 // of the results, you should use MaybeLookupLocationContext.
-func (s *Session) LookupLocationContext(ctx context.Context) (*enginelocate.Results, error) {
+func (s *Session) doLookupLocationContext(ctx context.Context) (*enginelocate.Results, error) {
 	task := enginelocate.NewTask(enginelocate.Config{
 		Logger:    s.Logger(),
 		Resolver:  s.resolver,
@@ -671,7 +671,7 @@ func (s *Session) lookupLocationContext(ctx context.Context) (*enginelocate.Resu
 	if s.testLookupLocationContext != nil {
 		return s.testLookupLocationContext(ctx)
 	}
-	return s.LookupLocationContext(ctx)
+	return s.doLookupLocationContext(ctx)
 }
 
 // MaybeLookupLocationContext is like MaybeLookupLocation but with a context

pkg/oonimkall/session.go

@@ -277,15 +277,14 @@ type GeolocateResults struct {
 func (sess *Session) Geolocate(ctx *Context) (*GeolocateResults, error) {
 	sess.mtx.Lock()
 	defer sess.mtx.Unlock()
-	info, err := sess.sessp.LookupLocationContext(ctx.ctx)
-	if err != nil {
+	if err := sess.sessp.MaybeLookupLocationContext(ctx.ctx); err != nil {
 		return nil, err
 	}
 	return &GeolocateResults{
-		ASN:     info.ASNString(),
-		Country: info.CountryCode,
-		IP:      info.ProbeIP,
-		Org:     info.NetworkName,
+		ASN:     sess.sessp.ProbeASNString(),
+		Country: sess.sessp.ProbeCC(),
+		IP:      sess.sessp.ProbeIP(),
+		Org:     sess.sessp.ProbeNetworkName(),
 	}, nil
 }
 
@@ -446,8 +445,7 @@ func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo,
 	if config.WebConnectivity == nil {
 		return nil, errors.New("oonimkall: missing webconnectivity config")
 	}
-	info, err := sess.sessp.LookupLocationContext(ctx.ctx)
-	if err != nil {
+	if err := sess.sessp.MaybeLookupLocationContext(ctx.ctx); err != nil {
 		return nil, err
 	}
 	if sess.TestingCheckInBeforeNewProbeServicesClient != nil {
@@ -464,8 +462,8 @@ func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo,
 		Charging:        config.Charging,
 		OnWiFi:          config.OnWiFi,
 		Platform:        config.Platform,
-		ProbeASN:        info.ASNString(),
-		ProbeCC:         info.CountryCode,
+		ProbeASN:        sess.sessp.ProbeASNString(),
+		ProbeCC:         sess.sessp.ProbeCC(),
 		RunType:         model.RunType(config.RunType),
 		SoftwareName:    config.SoftwareName,
 		SoftwareVersion: config.SoftwareVersion,

pkg/oonimkall/session_integration_test.go

@@ -297,7 +297,7 @@ func TestCheckInLookupLocationFailure(t *testing.T) {
 	config.WebConnectivity.AddCategory("CULTR")
 	ctx.Cancel() // immediate failure
 	result, err := sess.CheckIn(ctx, &config)
-	if !errors.Is(err, enginelocate.ErrAllIPLookuppersFailed) {
+	if !errors.Is(err, context.Canceled) {
 		t.Fatalf("not the error we expected: %+v", err)
 	}
 	if result != nil {