Add DisableWaitToShareSession

psiphon/common/inproxy/brokerClient.go

@@ -255,7 +255,13 @@ func (b *BrokerClient) roundTrip(
 	// session handshake, InitiatorSessions.RoundTrip will await completion
 	// of that handshake before sending the application-layer request.
 	//
-	// Note the waitToShareSession limitation, documented in
+	// This waitToShareSession functionality may be disabled with
+	// BrokerDialCoordinator.DisableWaitToShareSession, in which case
+	// concurrent Noise session handshakes may proceed; all concurrent
+	// sessions/requests may complete successfully and the last session to be
+	// established is retained by InitiatorSessions for reuse.
+	//
+	// Note the waitToShareSession isReadyToShare limitation, documented in
 	// InitiatorSessions.RoundTrip: a new session must complete a full,
 	// application-level round trip (e.g., ProxyAnnounce/ClientOffer), not
 	// just the session handshake, before a session becomes ready to share.
@@ -276,7 +282,7 @@ func (b *BrokerClient) roundTrip(
 	// Any time spent blocking on waitToShareSession is not included in
 	// requestDelay or requestTimeout.
 
-	waitToShareSession := true
+	waitToShareSession := !b.coordinator.DisableWaitToShareSession()
 
 	sessionHandshakeTimeout := common.ValueOrDefault(
 		b.coordinator.SessionHandshakeRoundTripTimeout(),

psiphon/common/inproxy/coordinator.go

@@ -119,6 +119,28 @@ type BrokerDialCoordinator interface {
 	// compartment ID.
 	PersonalCompartmentIDs() []ID
 
+	// DisableWaitToShareSession indicates whether to disable
+	// waitToShareSession, where concurrent broker client requests will wait
+	// for an in-flight Noise session handshake to complete before
+	// proceeding. Without waitToShareSession, multiple Noise session
+	// handshakes may be performed concurrently, with the last session
+	// retained for reuse. See the comment in BrokerClient.roundTrip, which
+	// describes waitToShareSession and its isReadyToShare limitation.
+	//
+	// In general, waitToShareSession will reduce broker Noise session
+	// overhead when there are many concurrent requests and no established
+	// session. In certain conditions, waitToShareSession might be faster, if
+	// a request waits briefly for another in-flight session establishment.
+	//
+	// Due to the isReadyToShare limitation, it is expected to be more optimal
+	// to disable waitToShareSession for in-proxy clients, so that multiple
+	// concurrent INPROXY tunnel protocol dials don't serialize, with all but
+	// the first dial awaiting the completion of the first dial's ClientOffer
+	// round trip, including the ProxyAnswer. For proxies, it remains
+	// preferable to use waitToShareSession, meaning that an initial
+	// ProxyAnnounce must complete before others will launch.
+	DisableWaitToShareSession() bool
+
 	// BrokerClientPrivateKey is the client or proxy's private key to be used
 	// in the secure session established with a broker. Clients should
 	// generate ephemeral keys; this is done automatically when a zero-value

psiphon/common/inproxy/coordinator_test.go

@@ -40,6 +40,7 @@ type testBrokerDialCoordinator struct {
 	networkType                       NetworkType
 	commonCompartmentIDs              []ID
 	personalCompartmentIDs            []ID
+	disableWaitToShareSession         bool
 	brokerClientPrivateKey            SessionPrivateKey
 	brokerPublicKey                   SessionPublicKey
 	brokerRootObfuscationSecret       ObfuscationSecret
@@ -85,6 +86,12 @@ func (t *testBrokerDialCoordinator) PersonalCompartmentIDs() []ID {
 	return t.personalCompartmentIDs
 }
 
+func (t *testBrokerDialCoordinator) DisableWaitToShareSession() bool {
+	t.mutex.Lock()
+	defer t.mutex.Unlock()
+	return t.disableWaitToShareSession
+}
+
 func (t *testBrokerDialCoordinator) BrokerClientPrivateKey() SessionPrivateKey {
 	t.mutex.Lock()
 	defer t.mutex.Unlock()

psiphon/common/inproxy/inproxy_test.go

@@ -776,7 +776,8 @@ func runTestInproxy(doMustUpgrade bool) error {
 		}
 	}
 
-	newClientBrokerClient := func() (*BrokerClient, error) {
+	newClientBrokerClient := func(
+		disableWaitToShareSession bool) (*BrokerClient, error) {
 
 		clientPrivateKey, err := GenerateSessionPrivateKey()
 		if err != nil {
@@ -789,6 +790,8 @@ func runTestInproxy(doMustUpgrade bool) error {
 
 			commonCompartmentIDs: testCommonCompartmentIDs,
 
+			disableWaitToShareSession: disableWaitToShareSession,
+
 			brokerClientPrivateKey:      clientPrivateKey,
 			brokerPublicKey:             brokerPublicKey,
 			brokerRootObfuscationSecret: brokerRootObfuscationSecret,
@@ -878,7 +881,12 @@ func runTestInproxy(doMustUpgrade bool) error {
 		return webRTCCoordinator, nil
 	}
 
-	sharedBrokerClient, err := newClientBrokerClient()
+	sharedBrokerClient, err := newClientBrokerClient(false)
+	if err != nil {
+		return errors.Trace(err)
+	}
+
+	sharedBrokerClientDisableWait, err := newClientBrokerClient(true)
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -894,9 +902,14 @@ func runTestInproxy(doMustUpgrade bool) error {
 
 		// Exercise BrokerClients shared by multiple clients, but also create
 		// several broker clients.
-		brokerClient := sharedBrokerClient
-		if i%2 == 0 {
-			brokerClient, err = newClientBrokerClient()
+		var brokerClient *BrokerClient
+		switch i % 3 {
+		case 0:
+			brokerClient = sharedBrokerClient
+		case 1:
+			brokerClient = sharedBrokerClientDisableWait
+		case 2:
+			brokerClient, err = newClientBrokerClient(true)
 			if err != nil {
 				return errors.Trace(err)
 			}

psiphon/common/parameters/parameters.go

@@ -488,6 +488,8 @@ const (
 	InproxyProxyIncompatibleNetworkTypes               = "InproxyProxyIncompatibleNetworkTypes"
 	InproxyClientIncompatibleNetworkTypes              = "InproxyClientIncompatibleNetworkTypes"
 	InproxyReplayRetainFailedProbability               = "InproxyReplayRetainFailedProbability"
+	InproxyProxyDisableWaitToShareSession              = "InproxyProxyDisableWaitToShareSession"
+	InproxyClientDisableWaitToShareSession             = "InproxyClientDisableWaitToShareSession"
 	InproxyEnableProxyQuality                          = "InproxyEnableProxyQuality"
 	InproxyEnableProxyQualityClientRegions             = "InproxyEnableProxyQualityClientRegions"
 	InproxyProxyQualityTargetUpstreamBytes             = "InproxyProxyQualityTargetUpstreamBytes"
@@ -1057,6 +1059,8 @@ var defaultParameters = map[string]struct {
 	InproxyProxyIncompatibleNetworkTypes:               {value: []string{}},
 	InproxyClientIncompatibleNetworkTypes:              {value: []string{}},
 	InproxyReplayRetainFailedProbability:               {value: 1.0, minimum: 0.0},
+	InproxyProxyDisableWaitToShareSession:              {value: false},
+	InproxyClientDisableWaitToShareSession:             {value: true},
 
 	InproxyEnableProxyQuality:                        {value: false, flags: serverSideOnly},
 	InproxyEnableProxyQualityClientRegions:           {value: []string{}, flags: serverSideOnly},

psiphon/inproxy.go

@@ -315,6 +315,7 @@ type InproxyBrokerClientInstance struct {
 	roundTripper                  *InproxyBrokerRoundTripper
 	personalCompartmentIDs        []inproxy.ID
 	commonCompartmentIDs          []inproxy.ID
+	disableWaitToShareSession     bool
 	sessionHandshakeTimeout       time.Duration
 	announceRequestTimeout        time.Duration
 	announceDelay                 time.Duration
@@ -581,6 +582,30 @@ func NewInproxyBrokerClientInstance(
 		b.retryOnFailedPeriod = p.Duration(parameters.InproxyProxyOnBrokerClientFailedRetryPeriod)
 	}
 
+	// Limitation: currently, disableWaitToShareSession is neither replayed
+	// nor is the selected value reported in metrics. The default tactics
+	// parameters are considered to be optimal: the in-proxy clients
+	// disabling wait and proxies using wait. The tactics flag can be used to
+	// enable wait for clients in case performance is poor or load on
+	// brokers -- due to simultaneous sessions -- is unexpectedly high.
+	//
+	// Note that, for broker dial parameter replay, the isValidReplay function
+	// currently invalidates replay only when broker specs change, and not
+	// when tactics in general change; so changes to these
+	// disableWaitToShareSession parameters would not properly invalidate
+	// replays in any case.
+	//
+	// Potential future enhancements for in-proxy client broker clients
+	// include using a pool of broker clients, with each one potentially
+	// using a different broker and/or fronting spec. In this scenario,
+	// waitToShareSession would be less impactful.
+
+	if isProxy {
+		b.disableWaitToShareSession = p.Bool(parameters.InproxyProxyDisableWaitToShareSession)
+	} else {
+		b.disableWaitToShareSession = p.Bool(parameters.InproxyClientDisableWaitToShareSession)
+	}
+
 	// Adjust long-polling request timeouts to respect any maximum request
 	// timeout supported by the provider fronting the request.
 	maxRequestTimeout, ok := p.KeyDurations(
@@ -849,6 +874,11 @@ func (b *InproxyBrokerClientInstance) PersonalCompartmentIDs() []inproxy.ID {
 	return b.personalCompartmentIDs
 }
 
+// Implements the inproxy.BrokerDialCoordinator interface.
+func (b *InproxyBrokerClientInstance) DisableWaitToShareSession() bool {
+	return b.disableWaitToShareSession
+}
+
 // Implements the inproxy.BrokerDialCoordinator interface.
 func (b *InproxyBrokerClientInstance) BrokerClientPrivateKey() inproxy.SessionPrivateKey {
 	return b.brokerClientPrivateKey