Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 11, 2025

LiftWriter in SIL.DictionaryServices appears to leak several MB per use; investigation and fix are needed.

  • Implemented disposal of temporary XmlReader/StringReader instances created during language form validation to eliminate per-call resource leaks.
  • Awaiting approval to address the remaining potential XmlWriter lifetime/Disposed-state issue.

No additional code paths changed beyond the temporary reader disposal; further remediation pending approval.

Original prompt

We are using LiftWriter from the SIL.DictionaryServices package and it seems to leak a few megabytes of memory every time it is used.

Make a plan for analyzing the code for leaks and wait before approval before beginning.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.


This change is Reviewable

Copilot AI changed the title [WIP] Analyze code for memory leaks in LiftWriter usage Plan: analyze and remediate LiftWriter memory growth Dec 11, 2025
Copilot AI requested a review from jasonleenaylor December 11, 2025 19:32
@jasonleenaylor
Copy link
Contributor

@copilot Do the inspection step and report on the results. Wait for interaction before continuing.

Copy link
Contributor Author

Copilot AI commented Dec 11, 2025

@copilot Do the inspection step and report on the results. Wait for interaction before continuing.

Inspection complete. Findings: LiftWriter owns only an XmlWriter; End closes it but does not set Disposed or clear the reference, so repeated End without Dispose leaves the object holding its writer until GC (no finalizer in release), which could explain transient memory growth if callers skip Dispose. Dispose(bool) closes the writer, but VerifyNotDisposed is inverted (throws when not disposed), so it isn’t usable to guard against improper reuse. Also noticed small resource hygiene gaps: per-Add language form validation creates XmlReader/StringReader instances without disposing them. No code changes made yet—let me know if you want me to proceed with a concrete repro harness and fixes (e.g., dispose/clear writer, correct Disposed semantics, dispose temp readers).

@jasonleenaylor
Copy link
Contributor

@copilot Show the XmlReader/StringReader instance leak and a proposed solution, wait for approval before making a commit.

@jasonleenaylor
Copy link
Contributor

@copilot Make the proposed change and commit it. Then show in context the possible XmlWriter leak and a proposed fix. Wait for approval before committing.

Co-authored-by: jasonleenaylor <2295227+jasonleenaylor@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 11, 2025

@copilot Make the proposed change and commit it. Then show in context the possible XmlWriter leak and a proposed fix. Wait for approval before committing.

XmlReader/StringReader disposal fixed in 87036ce (WriteLanguageFormsInWrapper now wraps the XmlReader creation in nested using blocks). Possible XmlWriter leak: End() flushes/closes the writer but never marks Disposed or releases the reference, so callers that only call End() keep a live LiftWriter holding a closed XmlWriter until GC. Proposed fix (pending approval): have End() call Dispose(true) (or at least _writer?.Dispose(); Disposed = true;) and adjust VerifyNotDisposed to check if (Disposed) throw new ObjectDisposedException(...), so the writer is properly disposed and future misuse is prevented without waiting for GC. No further commits made yet; ready to implement once approved.

@github-actions
Copy link

github-actions bot commented Dec 11, 2025

Palaso Tests

     3 files   -     1       3 suites   - 1   8m 19s ⏱️ - 2m 30s
 5 092 tests ±    0   4 859 ✅ ±    0  233 💤 ± 0  0 ❌ ±0 
14 761 runs   - 1 830  14 128 ✅  - 1 743  633 💤  - 87  0 ❌ ±0 

Results for commit 80c93da. ± Comparison against base commit d010103.

♻️ This comment has been updated with latest results.

@jasonleenaylor jasonleenaylor changed the title Plan: analyze and remediate LiftWriter memory growth Fix memory leak in LiftWriter Dec 11, 2025
@jasonleenaylor jasonleenaylor marked this pull request as ready for review December 11, 2025 20:11
@jasonleenaylor jasonleenaylor enabled auto-merge (squash) December 11, 2025 20:12
@jasonleenaylor
Copy link
Contributor

The other suggestion was not likely to be the cause so I've ignored it for now.

@imnasnainaec
Copy link
Contributor

@copilot Add a very brief summary of this fix to the "Fixed" subsection of the "Unreleased" section of CHANGELOG.md.

Copy link
Contributor

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

@imnasnainaec reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jasonleenaylor)

@jasonleenaylor jasonleenaylor merged commit 456f379 into master Dec 15, 2025
15 of 16 checks passed
@jasonleenaylor jasonleenaylor deleted the copilot/analyze-memory-leaks-liftwriter branch December 15, 2025 18:30
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