Clear child modules on dispose.

[joshprzybyszewski-wf]

Problem

We're seeing modules get leaking text_doc_client.

Solution

We think that clearing the list of modules is a fail-safe solution to ensure that we don't retain references to children modules when we've been disposed.

@jacobjohnson-wf @evanweible-wf @bencampbell-wf

[aviary-wf]

Security Insights

No security relevant content was detected by automated scans.

Action Items

• Review PR for security impact; comment "security review required" if needed or unsure
• Verify aviary.yaml coverage of security relevant code

---

Questions or Comments? Reach out on Slack: #support-infosec.

[codecov-io]

Codecov Report

Merging #139 into master will decrease coverage by 0.26%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
- Coverage   96.82%   96.55%   -0.27%     
==========================================
  Files           4        4              
  Lines         346      348       +2     
==========================================
+ Hits          335      336       +1     
- Misses         11       12       +1

Impacted Files	Coverage Δ
lib/src/lifecycle_module.dart	96.37% <75%> (-0.29%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43d8c70...c79ed2e. Read the comment docs.

[evanweible-wf]

We should add a test for this. I think the easiest way to do that would be to insert a tearDown step right here:
https://github.com/Workiva/w_module/blob/master/test/lifecycle_module_test.dart#L1502

tearDown(() {
  if (withChild) {
    expect(module.childModules, isEmpty);
  }
});

[todbachman-wf]

I would have expected to see this right after line 888 where we are disposing of the child modules. It seems like you guys have found a bug that is preventing us from hitting line 888 where we dispose of the child modules--is anyone trying to figure out why that's not getting hit?

[greglittlefield-wf]

See https://github.com/Workiva/datatables/pull/7354#pullrequestreview-184397831

[joshprzybyszewski-wf]

Hold on this; this might be a good change, but we might have isolated our problem to a different location.

[bencampbell-wf]

We remove child modules on disposal of the child module. I would suggest that we remove child modules on the unload of the child module as well.

w_module/lib/src/lifecycle_module.dart

Line 518 in f8b3602

_childModules.remove(childModule);

[greglittlefield-wf]

Just one memory-related comment.

Outdated diff

[evanweible-wf]

@joshprzybyszewski-wf @jacobjohnson-wf would you guys be able to use this branch to verify that it fixes the memory leaks you were seeing? That would serve as a good QA for this PR

[evanweible-wf]

^ nevermind, looking at the DT PR again, it looks like the leak would be addressed without this PR anyway, so it's not a great candidate for QA here.

[greglittlefield-wf]

• Changes look good
• All cases are covered by unit tests
• CI passes, and tests verify that children are neither retained via _childModules and arre disposed properly

QA +1

@Workiva/release-management-pp