Opened 19 months ago

Last modified 7 weeks ago

#25598 needs_revision enhancement

Let the broker inform proxies how often to poll

Reported by: dcf Owned by:
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords: starter
Cc: dcf, arlolra, Samdney, cohosh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Currently, proxies poll the broker at a static rate of once every 5–10 seconds. If we're anticipating thousands of proxies, we don't need them to poll so frequently.

The broker could instead tell each proxy how long to wait before polling again. The broker could even dynamically adjust the rate based on an estimate of supply and demand.

One way to do this would be a custom header in responses to /proxy requests:

Snowflake-Next-Poll: Thu, 22 Mar 2018 18:05:47 GMT

Or using a relative time offset:

Snowflake-Next-Poll: 600

There was a similar idea for flash proxy.

#8171
The facilitator included a fixed check-back-in=600 in its responses.
#8172
Adjust polling interval dynamically (never implemented).

Child Tickets

Change History (9)

comment:1 Changed 18 months ago by Samdney

Keywords: starter added

comment:2 Changed 18 months ago by Samdney

Cc: Samdney added

comment:3 in reply to:  description ; Changed 2 months ago by serna

I can do this.

The broker could instead tell each proxy how long to wait before polling again. The broker could even dynamically adjust the rate based on an estimate of supply and demand.


What information would you use to determine the time dynamically? I was thinking in using clientDeniedCount and proxyIdleCount from the metrics but it resets every 24 hours which can be a bit misleading.

One way to do this would be a custom header in responses to /proxy requests:

Snowflake-Next-Poll: Thu, 22 Mar 2018 18:05:47 GMT

Or using a relative time offset:

Snowflake-Next-Poll: 600


The second one seems like the best option. A timestamp can be confusing to read and would depend on the operating system clock which can be offset.

comment:4 in reply to:  3 Changed 8 weeks ago by dcf

Replying to serna:

The broker could instead tell each proxy how long to wait before polling again. The broker could even dynamically adjust the rate based on an estimate of supply and demand.


What information would you use to determine the time dynamically? I was thinking in using clientDeniedCount and proxyIdleCount from the metrics but it resets every 24 hours which can be a bit misleading.

The first step is just to use a hardcoded value of 20 s. Ignore clientDeniedCount and proxyIdleCount and everything else. Just call a function that returns a constant 20. Deciding on what logic should go in that function later is a task of its own.

comment:5 Changed 8 weeks ago by serna

Here's how it would work with a hardcoded 20sec poll rate: https://github.com/BubuAnabelas/snowflake/pull/1

The broker sends the Snowflake-Next-Poll header and the proxy reads it set it as it's next poll interval.

comment:6 in reply to:  5 ; Changed 8 weeks ago by dcf

Status: newneeds_revision

Replying to serna:

Here's how it would work with a hardcoded 20sec poll rate: https://github.com/BubuAnabelas/snowflake/pull/1

Thanks for this patch.

https://github.com/BubuAnabelas/snowflake/pull/1/files#diff-ba4bd8a4477426567c409d66c2cf8a28R175-R176

+			nextPoll, _ := strconv.Atoi(resp.Header.Get("Snowflake-Next-Poll"))
+			pollInterval = time.Duration(nextPoll) * time.Second

The Atoi needs an error check. Otherwise a parse error or missing header may return a value of 0 and make the proxy start trying to DoS the broker :)

There should also be a safety minimum, so the proxy will not obey if the broker asks it to go too fast. Cf. the corresponding code in flashproxy.

I propose factoring out a separate function that takes a string and returns a time.Duration. It parses the string and enforces a minimum. In case of a parse error, it returns a default.

https://github.com/BubuAnabelas/snowflake/pull/1/files#diff-ba4bd8a4477426567c409d66c2cf8a28L29-R30

-const pollInterval = 5 * time.Second
+var pollInterval = 20 * time.Second

Now that this is variable, it is better to limit its scope to the context in which it is used (not a global variable if possible). The constant default value can remain global.

https://github.com/BubuAnabelas/snowflake/pull/1/files#diff-177f47700613253ab3ba906035e86714R49

        snowflake.config.brokerPollInterval = xhr.getResponseHeader('Snowflake-Next-Poll') * 1000 || snowflake.config.brokerPollInterval;

It looks like the check for a missing header needs to go before the multiplication by 1000, not after. Here too I recommend factoring out a function that checks the syntax and enforces a minimum. (Think of the proxy trying to defend itself from a malfunctioning broker.)

It's better if the code doesn't modify things in the global Config object. The more you can localize the poll interval, the better.

comment:7 in reply to:  6 Changed 8 weeks ago by serna

Replying to dcf:

Thank you for the suggestions, since it's my first contribution I was missing out a lot of things.

Will make the suggested changes!

comment:8 Changed 7 weeks ago by cohosh

Cc: cohosh added

comment:9 in reply to:  5 Changed 7 weeks ago by cohosh

Replying to serna:

Here's how it would work with a hardcoded 20sec poll rate: https://github.com/BubuAnabelas/snowflake/pull/1

The broker sends the Snowflake-Next-Poll header and the proxy reads it set it as it's next poll interval.

Thank you serna for looking at this!

This ticket is related to how proxies and the broker interact (you've already commented on #29207) and also #30704 which discusses how to make changes between the broker and the proxy backwards compatible. The changes proposed here don't have any backwards compatibility issues so will work fine as proposed, but as mentioned in #30704, we might eventually want to bundle everything into the body in case the mechanism for signaling changes from HTTP to something else. This is more likely for the client interactions with the broker than the proxy, but who knows :)

We can always move this header to the body later with the other changes in #29207, but i'd suggest for now to make sure that the proxy behaves properly if the header is missing. It looks like dcf already mentioned a change to handle this, the call to getResponseHeader returns null if the header doesn't exist and so this check should be separated from the calculation and if the header doesn't exist the proxy should behave normally with the default poll period.

Thanks again for the great work \o/

Note: See TracTickets for help on using tickets.