Parallel container startup with deferred values

[xandris]

Hi dig maintainers,

My organization made an internal tool that has many different startup tasks that all produce a unique Go type. Before dig/fx, some tasks wrote to a shared struct, some tasks read from it, and some did both. This meant having to carefully order functions, keeping in mind these implicit dependencies hidden in func bodies. I'm busy removing this shared struct and rewriting each task as a dig/fx provider and letting dig order things automatically. It's wonderful! Can't thank you enough.

However—and sorry for frontloading this so much—our existing code, poor quality as it is, does perform many slow tasks in parallel. Forcing these to happen in serial would push our execution time over our execution frequency. But dependency injection with dig is a perfect match with our codebase, so I would really like to work something out.

I don't believe the core of dig is incompatible with parallel execution. To prove this to myself, as a first pass I made paramList use an Errgroup to call all its children and wait for them to finish, and I made the store maps sync.Map. Where two goroutines reached the same constructorNode, I put a mutex to protect the called field. It worked but felt too simplistic and hard to control.

The approach I settled on lets all dig code run in one goroutine, and user-provided constructors can either run in the same goroutine (default) or in pooled/ephemeral goroutines. This keeps mutexes out of dig's data structures and lets users decide how much concurrency they want.

I tried this several ways, but this pull request is the cleanest way I found. It creates a new deferred type params and constructorNodes return. A deferred can be observed and resolved like a very simple Promise. Since params and constructors pass values indirectly via a Scope, deferred doesn't need much state.

I would very much like to get concurrent initialization into dig by any means; it doesn't have to be this pull request. I'm willing to any changes you deem necessary; let's work together!

Refs GO-1191

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

:white_check_mark: abhinav
:white_check_mark: EstebanOlmedo
:white_check_mark: xandris
:white_check_mark: sywhang
:x: Alexandra Parker

---

Alexandra Parker seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[xandris]

That said, the timing of this change was a bit unfortunate. As you might have noticed, we're deep in the middle of refactoring and cleanup to support decorations and replacements in the container. Apologies for the conflicts, but if you don't mind resolving them, we can work with you to get this merged.

Roger! I've started working on the merge conflicts and I'll do a force push once those are resolved. I have some training to do today, but after that I'll start running review comments to ground. Very happy we can work on this together!

One comment I have for the decorators impl–not that anyone asked–is that there seems to be a lot of code duplication between constructors and decorators. There are limits to DRY just by Go's nature but I wonder if there's opportunities here...

[abhinav]

One comment I have for the decorators impl–not that anyone asked–is that there seems to be a lot of code duplication between constructors and decorators. There are limits to DRY just by Go's nature but I wonder if there's opportunities here...

Feedback is absolutely welcome! Yes, there is room for DRYing up there. The codebase has some amount of historical debt which we're aware of, so we've been trying to find a good balance between "refactor it all until it's nice and clean" and actually getting fx.Replace working. Right now we're leaning towards deferring some of the non-essential clean-ups until afterwards.

[sywhang]

There are limits to DRY just by Go's nature but I wonder if there's opportunities here...

Definitely! Thanks for pointing that out. As @abhinav said, we're kind of pushing hard to get the fx.Replace feature out ASAP so there are definitely some debts being left around. We'll be revisiting those after we make fx-side changes.

[codecov[bot]]

Codecov Report

Merging #315 (58a2c64) into master (50965fd) will decrease coverage by 0.26%. The diff coverage is 95.41%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
- Coverage   98.34%   98.08%   -0.27%     
==========================================
  Files          21       25       +4     
  Lines        1450     1565     +115     
==========================================
+ Hits         1426     1535     +109     
- Misses         15       20       +5     
- Partials        9       10       +1     

Impacted Files	Coverage Δ
provide.go	100.00% <ø> (ø)
container.go	95.23% <75.00%> (-4.77%)	:arrow_down:
internal/promise/deferred.go	93.02% <93.02%> (ø)
param.go	97.49% <93.67%> (+0.11%)	:arrow_up:
decorate.go	99.00% <95.83%> (-1.00%)	:arrow_down:
constructor.go	97.61% <100.00%> (+0.64%)	:arrow_up:
internal/scheduler/parallel.go	100.00% <100.00%> (ø)
internal/scheduler/scheduler.go	100.00% <100.00%> (ø)
internal/scheduler/unbounded.go	100.00% <100.00%> (ø)
invoke.go	100.00% <100.00%> (ø)
... and 11 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[xandris]

Would you be willing to pin to your branch of Dig while we make more time for this on our end?

The risk assessment makes sense to me. Standing by; we can use module replacements until this is ready. What do you mean by "pin"?

[abhinav]

Excellent! Again, thanks for the PR.

What do you mean by "pin"?

Oops, I meant module replacements. "Pin to X" (as in, "pin to branch X" or "pin to version "Y") is what we've been using to generically refer to module replacements. I think maybe the phrase was more accurate before Go modules (with dep and glide) when we had to sometimes pin a package to a specific version to ensure it did not get upgraded.

[sywhang]

Hi @xandris , apologies for the delayed review. We had a few other priorities to take care of, and I just found time to get back to review this PR. I'll spend some time this week to test this internally as well in our codebase.

But before I start doing that, I see that this branch has gone out of sync with master by quite a bit, so let me first address the merge conflicts first. Thank you for the patience!

[xandris]

It's been a minute. How are things?