Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove dependency on the backoff library #17

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

marten-seemann
Copy link

This has bitten us before (see #13).
Really, there's no need for an external library to do a simple (jittered and capped) exponential backoff.

}
// Exponential increase of the interval with jitter:
// the new interval will be between 1.5x and 2.5x the old interval, capped at maxInterval.
if interval != maxInterval {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that backoff still jitters with randomness, even when it reached its max. I'm not worrying about it much though, just FYI.

@betamos
Copy link

betamos commented Sep 22, 2021

Since I have some recent context debugging this (and this popped up in my feed), I'll share my thoughts and facts I found.

  • Removing external dependencies is good.
  • The backoff module is responsibly maintained. The maintainer is very strict about API changes (e.g. here).
  • It can be difficult to write unit tests for business logic which sources time and randomness in business logic. The backoff module may be easier to mock.
  • It's probably easier to tune the params or algorithm of the backoff module if necessary in the future.
  • Apple uses exponential back-off for bonjour, capped at an hour. They worry a lot about multicast traffic. Also, they seem to be using it for both server and browser, whereas afaik we only use it for the browser..

@marten-seemann
Copy link
Author

The backoff module is responsibly maintained. The maintainer is very strict about API changes (e.g. here).

My motivation for getting rid of this package is not based on the maintenance style of this repo. I just don't like pulling in a lot of code for something I can myself in 3 LOC.

It can be difficult to write unit tests for business logic which sources time and randomness in business logic. The backoff module may be easier to mock.

True, but that only applies if you dependency-inject your backoff dependency (which we didn't), so there's no change here.

Apple uses exponential back-off for bonjour, capped at an hour. They worry a lot about multicast traffic. Also, they seem to be using it for both server and browser, whereas afaik we only use it for the browser..

Are you suggesting that the maximum of one minute that we've been using here is too low?

Copy link

@petar petar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@marten-seemann marten-seemann merged commit a3c4995 into master Sep 27, 2021
@marten-seemann marten-seemann deleted the remove-backoff-library branch September 27, 2021 20:44
@betamos
Copy link

betamos commented Sep 30, 2021

Are you suggesting that the maximum of one minute that we've been using here is too low?

Unfortunately there's a conflict between recency and traffic amount - the longer TTL and browse intervals the more likely your data is stale and the longer it takes to recover when laptops connect and disconnect from Wifi and so on. As excessive mDNS traffic is not a common problem today, I keep these timeouts shorter in my application, so the current number is good for me.

If traffic amount becomes a problem, we could research more prior art (e.g. Chromecast, Dropbox) for "industry standard parameters" or even look at integrating with OS services like Avahi and Bonjour (which manages a single browser for all clients), should they be available on the system.

Thanks for the merge!

@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants