From 92e6be9b035ca9801e1ddcf9974e9086b6f24f87 Mon Sep 17 00:00:00 2001 From: Amir Khan Date: Tue, 12 Nov 2024 12:41:46 -0500 Subject: [PATCH 1/3] Optimize regen.NewGenerator performance * regen.NewGenerator made expensive calls to regexp.String() that are only useful for debugging. These calls are now conditional on GeneratorArgs.Debug flag. --- psiphon/common/regen/internal_generator.go | 28 ++++++++++++++-------- psiphon/common/regen/regen.go | 6 +++++ 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/psiphon/common/regen/internal_generator.go b/psiphon/common/regen/internal_generator.go index 1eef69566..9a0e53861 100644 --- a/psiphon/common/regen/internal_generator.go +++ b/psiphon/common/regen/internal_generator.go @@ -122,21 +122,21 @@ func newGenerator(regexp *syntax.Regexp, args *GeneratorArgs) (generator *intern // Generator that does nothing. func noop(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, error) { - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, args.Debug), func() ([]byte, error) { return []byte{}, nil }}, nil } func opEmptyMatch(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, error) { enforceOp(regexp, syntax.OpEmptyMatch) - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, args.Debug), func() ([]byte, error) { return []byte{}, nil }}, nil } func opLiteral(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, error) { enforceOp(regexp, syntax.OpLiteral) - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, args.Debug), func() ([]byte, error) { if args.ByteMode { return runesToBytes(regexp.Rune...) } else { @@ -147,7 +147,7 @@ func opLiteral(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, func opAnyChar(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, error) { enforceOp(regexp, syntax.OpAnyChar) - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, args.Debug), func() ([]byte, error) { if args.ByteMode { return runesToBytes(rune(args.rng.Intn(math.MaxUint8 + 1))) } else { @@ -164,7 +164,7 @@ func opAnyCharNotNl(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenera } else { charClass = newCharClass(1, rune(math.MaxInt32)) } - return createCharClassGenerator(regexp.String(), charClass, args) + return createCharClassGenerator(regexpName(regexp, args.Debug), charClass, args) } func opQuest(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, error) { @@ -200,7 +200,7 @@ func opCharClass(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator } else { charClass = parseCharClass(regexp.Rune) } - return createCharClassGenerator(regexp.String(), charClass, args) + return createCharClassGenerator(regexpName(regexp, args.Debug), charClass, args) } func opConcat(regexp *syntax.Regexp, genArgs *GeneratorArgs) (*internalGenerator, error) { @@ -211,7 +211,7 @@ func opConcat(regexp *syntax.Regexp, genArgs *GeneratorArgs) (*internalGenerator return nil, generatorError(err, "error creating generators for concat pattern /%s/", regexp) } - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, genArgs.Debug), func() ([]byte, error) { var result bytes.Buffer for _, generator := range generators { gen, err := generator.Generate() @@ -234,7 +234,7 @@ func opAlternate(regexp *syntax.Regexp, genArgs *GeneratorArgs) (*internalGenera numGens := len(generators) - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, genArgs.Debug), func() ([]byte, error) { i := genArgs.rng.Intn(numGens) generator := generators[i] return generator.Generate() @@ -257,7 +257,7 @@ func opCapture(regexp *syntax.Regexp, args *GeneratorArgs) (*internalGenerator, // Group indices are 0-based, but index 0 is the whole expression. index := regexp.Cap - 1 - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, args.Debug), func() ([]byte, error) { return args.CaptureGroupHandler(index, regexp.Name, groupRegexp, generator, args) }}, nil } @@ -312,7 +312,7 @@ func createRepeatingGenerator(regexp *syntax.Regexp, genArgs *GeneratorArgs, min max = int(genArgs.MaxUnboundedRepeatCount) } - return &internalGenerator{regexp.String(), func() ([]byte, error) { + return &internalGenerator{regexpName(regexp, genArgs.Debug), func() ([]byte, error) { n := min + genArgs.rng.Intn(max-min+1) var result bytes.Buffer @@ -326,3 +326,11 @@ func createRepeatingGenerator(regexp *syntax.Regexp, genArgs *GeneratorArgs, min return result.Bytes(), nil }}, nil } + +// regexpName returns `regexp.String()` only if `debug` is true. +func regexpName(regexp *syntax.Regexp, debug bool) string { + if debug { + return regexp.String() + } + return "" +} diff --git a/psiphon/common/regen/regen.go b/psiphon/common/regen/regen.go index 69737d276..90abd9134 100644 --- a/psiphon/common/regen/regen.go +++ b/psiphon/common/regen/regen.go @@ -162,6 +162,9 @@ type GeneratorArgs struct { // ByteMode is not compatible with negated character classes (e.g. "[^a]"). ByteMode bool + // Debug is to used by the generator to log extra information. + Debug bool + // Used by generators. rng *rand.Rand } @@ -209,6 +212,9 @@ func (a *GeneratorArgs) Rng() (*rand.Rand, error) { // Generator generates random bytes or strings. type Generator interface { Generate() ([]byte, error) + + // String returns a string representation of the generator for debugging. + // Value is empty string if Debug is false. String() string } From 87d1a79696fdfeda85655c80c0274b7c795904b9 Mon Sep 17 00:00:00 2001 From: Rod Hynes Date: Wed, 13 Nov 2024 09:52:02 -0500 Subject: [PATCH 2/3] Update NetworkIDGetter comment --- psiphon/net.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/psiphon/net.go b/psiphon/net.go index 8b35d2641..d7d590936 100644 --- a/psiphon/net.go +++ b/psiphon/net.go @@ -156,7 +156,7 @@ type HasIPv6RouteGetter interface { // provider, which returns an identifier for the host's current active // network. // -// The identifier is a string that should indicate the network type and +// The identifier is a string that indicates the network type and // identity; for example "WIFI-" or "MOBILE-". As this network // ID is personally identifying, it is only used locally in the client to // determine network context and is not sent to the Psiphon server. The @@ -164,12 +164,21 @@ type HasIPv6RouteGetter interface { // substring before the first "-" is logged, so all PII must appear after the // first "-". // -// NetworkIDGetter.GetNetworkID should always return an identifier value, as +// NetworkIDGetter.GetNetworkID must always return an identifier value, as // logic that uses GetNetworkID, including tactics, is intended to proceed -// regardless of whether an accurate network identifier can be obtained. By -// convention, the provider should return "UNKNOWN" when an accurate network +// regardless of whether an accurate network identifier can be obtained. The +// the provider shall return "UNKNOWN" when an accurate network // identifier cannot be obtained. Best-effort is acceptable: e.g., return just // "WIFI" when only the type of the network but no details can be determined. +// +// The network type is sent to Psiphon servers and logged as +// server_tunnel.network_type. To ensure consistency in stats, all providers +// must use the same network type string values, currently consisting of: +// - "WIFI" for a Wi-Fi network +// - "MOBILE" for a mobile/cellular network +// - "WIRED" for a wired network +// - "VPN" for a VPN network +// - "UNKNOWN" for when the network type cannot be determined type NetworkIDGetter interface { GetNetworkID() string } From bce7b13b59872a1cd837cc3a40b41f21c9f39c50 Mon Sep 17 00:00:00 2001 From: Rod Hynes Date: Wed, 13 Nov 2024 09:58:14 -0500 Subject: [PATCH 3/3] Add tactic parameter to configure proxy max backoff delay - Fix: reset exponential backoff when proxyOneClient returns no error --- psiphon/common/inproxy/coordinator.go | 1 + psiphon/common/inproxy/coordinator_test.go | 7 +++++++ psiphon/common/inproxy/proxy.go | 22 +++++++++++----------- psiphon/common/parameters/parameters.go | 2 ++ psiphon/config.go | 5 +++++ psiphon/inproxy.go | 7 +++++++ 6 files changed, 33 insertions(+), 11 deletions(-) diff --git a/psiphon/common/inproxy/coordinator.go b/psiphon/common/inproxy/coordinator.go index d1c66c6d2..257ddea0d 100644 --- a/psiphon/common/inproxy/coordinator.go +++ b/psiphon/common/inproxy/coordinator.go @@ -182,6 +182,7 @@ type BrokerDialCoordinator interface { SessionHandshakeRoundTripTimeout() time.Duration AnnounceRequestTimeout() time.Duration AnnounceDelay() time.Duration + AnnounceMaxBackoffDelay() time.Duration AnnounceDelayJitter() float64 AnswerRequestTimeout() time.Duration OfferRequestTimeout() time.Duration diff --git a/psiphon/common/inproxy/coordinator_test.go b/psiphon/common/inproxy/coordinator_test.go index 28eebb49b..14eccc6c1 100644 --- a/psiphon/common/inproxy/coordinator_test.go +++ b/psiphon/common/inproxy/coordinator_test.go @@ -50,6 +50,7 @@ type testBrokerDialCoordinator struct { sessionHandshakeRoundTripTimeout time.Duration announceRequestTimeout time.Duration announceDelay time.Duration + announceMaxBackoffDelay time.Duration announceDelayJitter float64 answerRequestTimeout time.Duration offerRequestTimeout time.Duration @@ -149,6 +150,12 @@ func (t *testBrokerDialCoordinator) AnnounceDelay() time.Duration { return t.announceDelay } +func (t *testBrokerDialCoordinator) AnnounceMaxBackoffDelay() time.Duration { + t.mutex.Lock() + defer t.mutex.Unlock() + return t.announceMaxBackoffDelay +} + func (t *testBrokerDialCoordinator) AnnounceDelayJitter() float64 { t.mutex.Lock() defer t.mutex.Unlock() diff --git a/psiphon/common/inproxy/proxy.go b/psiphon/common/inproxy/proxy.go index 3a6427fe7..3a16d99e3 100644 --- a/psiphon/common/inproxy/proxy.go +++ b/psiphon/common/inproxy/proxy.go @@ -272,13 +272,14 @@ loop: // proxyOneClient fails. As having no broker clients is a possible // proxyOneClient failure case, GetBrokerClient errors are ignored here and // defaults used in that case. -func (p *Proxy) getAnnounceDelayParameters() (time.Duration, float64) { +func (p *Proxy) getAnnounceDelayParameters() (time.Duration, time.Duration, float64) { brokerClient, err := p.config.GetBrokerClient() if err != nil { - return proxyAnnounceDelay, proxyAnnounceDelayJitter + return proxyAnnounceDelay, proxyAnnounceMaxBackoffDelay, proxyAnnounceDelayJitter } brokerCoordinator := brokerClient.GetBrokerDialCoordinator() return common.ValueOrDefault(brokerCoordinator.AnnounceDelay(), proxyAnnounceDelay), + common.ValueOrDefault(brokerCoordinator.AnnounceMaxBackoffDelay(), proxyAnnounceMaxBackoffDelay), common.ValueOrDefault(brokerCoordinator.AnnounceDelayJitter(), proxyAnnounceDelayJitter) } @@ -384,6 +385,10 @@ func (p *Proxy) proxyClients( backOff, err := p.proxyOneClient( ctx, logAnnounce, signalAnnounceDone) + if !backOff || err == nil { + failureDelayFactor = 1 + } + if err != nil && ctx.Err() == nil { // Apply a simple exponential backoff based on whether @@ -396,17 +401,12 @@ func (p *Proxy) proxyClients( // prevents both excess local logging and churning in the former // case and excessive bad service to clients or unintentionally // overloading the broker in the latter case. - // - // TODO: specific tactics parameters to control this logic. - delay, jitter := p.getAnnounceDelayParameters() + delay, maxBackoffDelay, jitter := p.getAnnounceDelayParameters() - if !backOff { - failureDelayFactor = 1 - } delay = delay * failureDelayFactor - if delay > proxyAnnounceMaxBackoffDelay { - delay = proxyAnnounceMaxBackoffDelay + if delay > maxBackoffDelay { + delay = maxBackoffDelay } if failureDelayFactor < 1<<20 { failureDelayFactor *= 2 @@ -608,7 +608,7 @@ func (p *Proxy) proxyOneClient( // any deliberate delay. requestDelay := time.Duration(0) - announceDelay, announceDelayJitter := p.getAnnounceDelayParameters() + announceDelay, _, announceDelayJitter := p.getAnnounceDelayParameters() p.nextAnnounceMutex.Lock() nextDelay := prng.JitterDuration(announceDelay, announceDelayJitter) if p.nextAnnounceBrokerClient != brokerClient { diff --git a/psiphon/common/parameters/parameters.go b/psiphon/common/parameters/parameters.go index f23e2e6f3..3545604ee 100644 --- a/psiphon/common/parameters/parameters.go +++ b/psiphon/common/parameters/parameters.go @@ -411,6 +411,7 @@ const ( InproxyProxyAnnounceRequestTimeout = "InproxyProxyAnnounceRequestTimeout" InproxyProxyAnnounceDelay = "InproxyProxyAnnounceDelay" InproxyProxyAnnounceDelayJitter = "InproxyProxyAnnounceDelayJitter" + InproxyProxyAnnounceMaxBackoffDelay = "InproxyProxyAnnounceMaxBackoffDelay" InproxyProxyAnswerRequestTimeout = "InproxyProxyAnswerRequestTimeout" InproxyClientOfferRequestTimeout = "InproxyClientOfferRequestTimeout" InproxyClientOfferRequestPersonalTimeout = "InproxyClientOfferRequestPersonalTimeout" @@ -930,6 +931,7 @@ var defaultParameters = map[string]struct { InproxyProxyAnnounceRequestTimeout: {value: 2*time.Minute + 10*time.Second, minimum: time.Duration(0)}, InproxyProxyAnnounceDelay: {value: 100 * time.Millisecond, minimum: time.Duration(0)}, InproxyProxyAnnounceDelayJitter: {value: 0.5, minimum: 0.0}, + InproxyProxyAnnounceMaxBackoffDelay: {value: 1 * time.Minute, minimum: time.Duration(0)}, InproxyProxyAnswerRequestTimeout: {value: 10*time.Second + 10*time.Second, minimum: time.Duration(0)}, InproxyClientOfferRequestTimeout: {value: 10*time.Second + 10*time.Second, minimum: time.Duration(0)}, InproxyClientOfferRequestPersonalTimeout: {value: 5*time.Second + 10*time.Second, minimum: time.Duration(0)}, diff --git a/psiphon/config.go b/psiphon/config.go index 990874106..b972c2557 100755 --- a/psiphon/config.go +++ b/psiphon/config.go @@ -996,6 +996,7 @@ type Config struct { InproxyMaxCompartmentIDListLength *int InproxyProxyAnnounceRequestTimeoutMilliseconds *int InproxyProxyAnnounceDelayMilliseconds *int + InproxyProxyAnnounceMaxBackoffDelayMilliseconds *int InproxyProxyAnnounceDelayJitter *float64 InproxyProxyAnswerRequestTimeoutMilliseconds *int InproxyClientOfferRequestTimeoutMilliseconds *int @@ -2529,6 +2530,10 @@ func (config *Config) makeConfigParameters() map[string]interface{} { applyParameters[parameters.InproxyProxyAnnounceDelay] = fmt.Sprintf("%dms", *config.InproxyProxyAnnounceDelayMilliseconds) } + if config.InproxyProxyAnnounceMaxBackoffDelayMilliseconds != nil { + applyParameters[parameters.InproxyProxyAnnounceMaxBackoffDelay] = fmt.Sprintf("%dms", *config.InproxyProxyAnnounceMaxBackoffDelayMilliseconds) + } + if config.InproxyProxyAnnounceDelayJitter != nil { applyParameters[parameters.InproxyProxyAnnounceDelayJitter] = *config.InproxyProxyAnnounceDelayJitter } diff --git a/psiphon/inproxy.go b/psiphon/inproxy.go index ec00f023c..7d9b6336e 100644 --- a/psiphon/inproxy.go +++ b/psiphon/inproxy.go @@ -275,6 +275,7 @@ type InproxyBrokerClientInstance struct { sessionHandshakeTimeout time.Duration announceRequestTimeout time.Duration announceDelay time.Duration + announceMaxBackoffDelay time.Duration announceDelayJitter float64 answerRequestTimeout time.Duration offerRequestTimeout time.Duration @@ -520,6 +521,7 @@ func NewInproxyBrokerClientInstance( sessionHandshakeTimeout: p.Duration(parameters.InproxySessionHandshakeRoundTripTimeout), announceRequestTimeout: p.Duration(parameters.InproxyProxyAnnounceRequestTimeout), announceDelay: p.Duration(parameters.InproxyProxyAnnounceDelay), + announceMaxBackoffDelay: p.Duration(parameters.InproxyProxyAnnounceMaxBackoffDelay), announceDelayJitter: p.Float(parameters.InproxyProxyAnnounceDelayJitter), answerRequestTimeout: p.Duration(parameters.InproxyProxyAnswerRequestTimeout), offerRequestTimeout: p.Duration(parameters.InproxyClientOfferRequestTimeout), @@ -1007,6 +1009,11 @@ func (b *InproxyBrokerClientInstance) AnnounceDelay() time.Duration { return b.announceDelay } +// Implements the inproxy.BrokerDialCoordinator interface. +func (b *InproxyBrokerClientInstance) AnnounceMaxBackoffDelay() time.Duration { + return b.announceMaxBackoffDelay +} + // Implements the inproxy.BrokerDialCoordinator interface. func (b *InproxyBrokerClientInstance) AnnounceDelayJitter() float64 { return b.announceDelayJitter