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

Break BaseService functions into serviceutil #536

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Aug 20, 2024

I was going through trying to refactor the demo project to use
rivershared, and while doing so, noticed that it'd broken out some
of the BaseService functions like CancellableSleep into a new
package so that they could be used without an available BaseService.

Looking over this API again, there's no good reason for functions like
CancellableSleep and ExponentialBackoff to be tied so tight to
BaseService. The reason they are is that BaseService provides a
random source that both can conveniently use, but that can be moved to a
function argument instead.

The reason that BaseService has a random source is to avoid the old
math/rand seeding problem. A single random source is initialized and
seeding securely once, then placed onto an archetype that's inherited
by all BaseService instances.

Soon, even that can go away. One of the biggest things fixed by Go
1.22's math/rand/v2 is that it's no longer necessary to seed the top
level functions. Once we drop support for Go 1.21, we'll be able to
simplify this code dramatically by dropping BaseService.Rand and the
random source arguments from functions like ExponentialBackoff in
favor of just using top level math/rand/v2 functions.

I also drop the variant CancellableSleepBetween in favor of having
callers use CancellableSleep combined with randutil.DurationBetween,
a new random helper similar to IntBetween.

With all this in, we'll be able to fully jettison all utilities from the
demo project in favor of going all in with rivershared's equivalents.

@brandur brandur force-pushed the brandur-service-util branch 3 times, most recently from ed8b822 to 7235b00 Compare August 20, 2024 03:17
@@ -59,9 +59,6 @@ jobs:
- name: Display Go version
run: go version

- name: Install dependencies
run: go get -t ./...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A workspace side effect: adding a new package to rivershared seems to make it hard to resolve by go get -t despite go test and another commands working fine. No functional change by removing this since all Go commands will get dependencies, but another strike against workspaces, and a weird one.

I was going through trying to refactor the demo project to use
`rivershared`, and while doing so, noticed that it'd broken out some
of the `BaseService` functions like `CancellableSleep` into a new
package so that they could be used without an available `BaseService`.

Looking over this API again, there's no good reason for functions like
`CancellableSleep` and `ExponentialBackoff` to be tied so tight to
`BaseService`. The reason they are is that `BaseService` provides a
random source that both can conveniently use, but that can be moved to a
function argument instead.

The reason that `BaseService` has a random source is to avoid the old
`math/rand` seeding problem. A single random source is initialized and
seeding securely once, then placed onto an archetype that's inherited
by all `BaseService` instances.

Soon, even that can go away. One of the biggest things fixed by Go
1.22's `math/rand/v2` is that it's no longer necessary to seed the top
level functions. Once we drop support for Go 1.21, we'll be able to
simplify this code dramatically by dropping `BaseService.Rand` and the
random source arguments from functions like `ExponentialBackoff` in
favor of just using top level `math/rand/v2` functions.

I also drop the variant `CancellableSleepBetween` in favor of having
callers use `CancellableSleep` combined with `randutil.DurationBetween`,
a new random helper similar to `IntBetween`.

With all this in, we'll be able to fully jettison all utilities from the
demo project in favor of going all in with `rivershared`'s equivalents.
@brandur
Copy link
Contributor Author

brandur commented Aug 20, 2024

@bgentry With these changes I think we can get the demo app fully switched over to rivershared.

@brandur
Copy link
Contributor Author

brandur commented Aug 22, 2024

thx.

@brandur brandur merged commit 8f5ee36 into master Aug 22, 2024
14 checks passed
@brandur brandur deleted the brandur-service-util branch August 22, 2024 02:03
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.

2 participants