You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Make Surface implement IDisposable (API shape only, no pooling yet) - #16
This is an intentional, deliberate breaking API change: Surface (src/DemaConsulting.CanvasNet/Canvas/Surface.cs) now implements IDisposable.
Scope is deliberately narrow — API shape only, no pooling yet. This PR adds the IDisposable contract (a _disposed flag, an idempotent no-finalizer Dispose(), and ObjectDisposedException.ThrowIf(_disposed, this) guards on every public buffer-touching member) so that Surface's internal pixel storage can be switched to ArrayPool<byte>-backed rented arrays in a future, non-breaking PR. Plain-array-backed storage (new byte[...]) is completely unchanged in this PR; disposing a Surface today does not release/return anything, it purely flips the guard flag. Real ArrayPool pooling is intentionally deferred to a dedicated follow-up PR (it has its own correctness subtleties — e.g. ArrayPool<T>.Rent returning arrays larger than requested, and a security-relevant question about zeroing pixel data on return-to-pool — that deserve focused review, not to be bundled with this breaking API-shape change).
Why now
Adding IDisposable to Surface is a binary/source breaking change (any code holding a Surface in a using block today would newly be required to, though nothing is force-required by the compiler — see below). Making this change before 1.0 preserves the option of ArrayPool-based pooling post-1.0 without a second breaking change later.
What changed
Surface is now sealed class Surface : IDisposable.
ObjectDisposedException.ThrowIf(_disposed, this) guards added to all 11 public members that touch the internal pixel buffer (13 call-sites, counting the indexer's get/set and CompositeOver(Surface)'s self/foreground checks separately): the indexer get/set, GetRowSpanBytes, GetRowSpan, Crop, PremultiplyAlpha, UnpremultiplyAlpha, Clear, both CompositeOver overloads, and both public CompositeOverSpan overloads. The two internal workspace-reusing CompositeOverSpan overloads used by ScanlineRasterizer carry the same guard (15 guards total), so a disposed surface cannot be mutated through the internal fill path either.
GetRowSpanBytes/GetRowSpan gained an explicit span-lifetime contract in the design doc: the disposal guard only runs when those methods are called, so a caller retaining a previously-obtained span across a Dispose() call gets undefined behavior — callers must not do that.
Dispose() is idempotent (safe to call twice) and has no finalizer — matching the exact precedent already established by Surface.cs's own internal RowChannelBuffers/CompositeWorkBuffers helper types, since these are managed arrays, not unmanaged resources.
Canvas (src/DemaConsulting.CanvasNet/Rendering/Canvas.cs) does not become IDisposable and does not dispose the Surface it wraps — doc-only change adding explicit <remarks> to its constructor and Surface property stating this non-owning-reference contract (the caller that constructed the Surface remains responsible for disposing it).
Zero churn across the ~178 pre-existing new Surface(...) call sites in src//test/ — verified CA2000 is not a build-breaking diagnostic under this repo's analyzer settings, so none of them need to change to keep compiling/passing.
README.md and docs/user_guide/introduction.md code samples that construct a Surface now use the using var surface = new Surface(...) idiom, for consistency/teaching a good pattern.
Companion artifacts updated per software-items.md: reqstream requirements (docs/reqstream/canvas-net/canvas.yaml, docs/reqstream/canvas-net/canvas/surface.yaml), design docs (docs/design/canvas-net/canvas/surface.md, docs/design/canvas-net/rendering/canvas.md), verification docs (docs/verification/canvas-net/canvas/surface.md, docs/verification/canvas-net/rendering/canvas.md), and the SysML2 model note (docs/sysml2/model/canvas-net/canvas/surface.sysml).
One *_AfterDispose_ThrowsObjectDisposedException test per guarded call-site, plus the foreground-disposed case for CompositeOver(Surface) (15 tests)
Canvas_Surface_DisposedExternally_CanvasDoesNotThrowFromAccessingSurfaceProperty — a design-intent regression test documenting that Canvas continues to allow access to a disposed Surface without Canvas itself throwing.
CanvasNet_SystemIntegration_DisposeSurfaceThenUseIt_ThrowsObjectDisposedException — a system-level integration test proving the disposal contract is reachable end to end through the public API, using a representative sample of guarded members (the full member-by-member matrix lives in SurfaceTests).
Validation
pwsh ./fix.ps1 run.
Full build: 0 warnings/errors across net8.0/net9.0/net10.0 (TreatWarningsAsErrors=true).
Full test suite: 3810/3810 passing across net8.0/net9.0/net10.0 (1270 per TFM).
dotnet reqstream --lint, dotnet reviewmark --lint, SysML2 lint — all pass with 0 errors.
Follow-up (tracked separately, not in this PR)
ArrayPool-based pooling of Surface's internal buffer (the internal implementation of the constructor + Dispose()), including the rented-array-size and clear-on-return design questions noted above.
- Surface is now a sealed IDisposable class with an idempotent, non-finalizer
Dispose() (managed arrays only, no pooling yet).
- ObjectDisposedException.ThrowIf(_disposed, this) guards every public member
that touches the pixel buffer: indexer get/set, GetRowSpanBytes, GetRowSpan,
Crop, PremultiplyAlpha, UnpremultiplyAlpha, CompositeOver(Surface),
CompositeOver(Rgba32), and both public CompositeOverSpan overloads.
- Canvas gains doc-only remarks clarifying it holds a non-owning reference to
its Surface and does not dispose it; Canvas itself remains non-IDisposable.
- Adds 13 new SurfaceTests.cs tests (Dispose-once/twice plus one
*_AfterDispose_ThrowsObjectDisposedException test per guarded member) and one
CanvasTests.cs regression test documenting Canvas's non-owning behavior.
- Updates companion artifacts: reqstream requirements (canvas.yaml,
canvas/surface.yaml), design docs (canvas/surface.md,
rendering/canvas.md), verification docs (canvas/surface.md,
rendering/canvas.md), sysml2 model note, README.md, and
docs/user_guide/introduction.md (new Disposal subsection plus using var
conversions at all literal new Surface(...) call sites).
…al, and fix disposal wording/examples
- CompositeOver(Surface) now also throws ObjectDisposedException when the
foreground argument has been disposed, not just when this surface has.
- Add Surface_Clear_AfterDispose_ThrowsObjectDisposedException and
Surface_CompositeOverSurface_ForegroundDisposed_ThrowsObjectDisposedException,
and list both in the reqstream/verification test matrices.
- Reword the parent/child disposal requirements (canvas.yaml,
canvas/surface.yaml) to say Dispose logically releases the pixel buffer,
matching the plain-managed-array implementation instead of promising an
unmanaged/pooled resource release.
- Add Clear to the design doc's and user guide's enumerated
disposal-guarded-member lists.
- Bind the Crop result with using var in the README and user guide examples
so the samples model the lifetime contract for derived surfaces.
- Add finalizer to the cspell dictionary and fix a design-doc table
line-length/alignment lint failure.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The row accessors return mutable spans that alias _buffer, so a caller can retain a span, call Dispose(), and continue reading or writing the array; the guard only affects later member calls. If a follow-up returns this storage to ArrayPool here, that stale span could mutate another surface's reused array or expose prior pixels, so the stated future pooling change is not safely non-breaking. Please define a lifetime/lease strategy or explicitly prohibit returning this storage while aliases may exist.
This issue also appears on line 595 of the same file.
The newly added exception makes the preceding "this method never throws" statement inaccurate: every byte pattern is valid, but a disposed surface now throws ObjectDisposedException. Please qualify the sentence so the XML documentation does not contradict the exception contract.
This issue also appears on line 484 of the same file.
CanvasNet-Canvas-Disposal and its child CanvasNet-Canvas-Surface-Disposal
were unreachable from any system/quality-tagged requirement, so ReqStream's
enforce step reported them as orphans. Add CanvasNet-Lib-CanvasDisposal as
the missing system-level parent, following the same pattern as the sibling
CanvasNet-Lib-CanvasClear requirement.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assign the discarded value instead of a bare property-access expression
body, per review feedback, even though it already compiled (bare property
access isn't a valid Action statement, so overload resolution picked
Func<object>).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Correct inaccurate claims about idempotent disposal
docs/design/canvas-net/canvas/surface.md:339
The cited helpers do not use an idempotent flag check: their Dispose methods unconditionally return their rented arrays, so a second call can return the same arrays twice. This comparison makes the new disposal documentation inaccurate and could establish a false precedent for the future pooled Surface implementation; describe only Surface's flag-based behavior or correct the helper description.
Dispose all codec-loaded surfaces in the canonical sample
README.md:77
This canonical sample now teaches using for the directly constructed Surface, but the Surface instances returned by BmpCodec.Load, PngCodec.Load, TiffCodec.Load, JpegCodec.Load, SvgCodec.Load, and the later PngCodec.Load call are still left undisposed. Those results also own the pixel buffer and would retain pooled storage in the follow-up implementation; make every loaded surface using var (or dispose it before the sample ends) so the sample demonstrates the lifetime contract consistently.
Prevent helper disposal from returning buffers twice
docs/design/canvas-net/canvas/surface.md:339
The cited RowChannelBuffers and CompositeWorkBuffers do not follow the claimed idempotent flag-check/set/return shape: their Dispose methods unconditionally return each rented array, so a second call would return the same arrays twice. Remove this comparison or update the helper types before using them as the precedent for Surface disposal.
Dispose surfaces returned by codec load examples
docs/user_guide/introduction.md:93
The new disposal guidance says to dispose a Surface once it is no longer needed, but the load/save examples below leave every Surface returned by a codec undisposed (loaded, loaded24, loadedRgb, loadedLzw, the JPEG result, and the untrusted PNG result). Since codec loads also create the buffer whose future storage is intended to be pooled, update these examples to use using var for the returned surfaces as well.
…tion disposal test
- Add ObjectDisposedException guards to the internal workspace-reusing
CompositeOverSpan overloads used by ScanlineRasterizer, so a disposed
surface's buffer cannot be mutated through the internal fill path.
- Document span lifetime for GetRowSpanBytes/GetRowSpan: the disposal
guard only runs when those methods are called, so a caller holding a
previously-obtained span across Dispose() gets undefined behavior.
- Correct the design doc's inaccurate claim that RowChannelBuffers and
CompositeWorkBuffers use the same idempotent flag-checked disposal
shape as Surface; they do not.
- Add CanvasNet_SystemIntegration_DisposeSurfaceThenUseIt_ThrowsObjectDisposedException
and reference it from the root CanvasNet-Lib-CanvasDisposal requirement
instead of a unit-level test, matching sibling root requirements.
- Add unit tests for the two internal CompositeOverSpan workspace
overloads' disposed guards, and wire them into the reqstream/
verification test lists.
- Use 'using var' for all codec Load(...) results in README.md and
docs/user_guide/introduction.md so canonical samples don't leak
Surface instances.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The PR summary's inventory is now stale: Clear is also a public buffer-touching member and this guard plus Surface_Clear_AfterDispose_ThrowsObjectDisposedException were added. The stated “9 members/11 call-sites/11 tests” therefore omits Clear (and the foreground-disposed case is an additional test); please update the description's counts and member list to match the implementation.
…overage
- Note that the 'future pooling is non-breaking' claim in the design
doc is limited to the API surface: a future pooled implementation
must still honor the span-lifetime rule, since Dispose() cannot
retroactively invalidate a span obtained before it was called.
- Reword the CanvasNet_SystemIntegration_DisposeSurfaceThenUseIt_...
test summary to describe it as exercising a representative sample
of guarded members rather than overclaiming full member coverage;
the complete matrix lives in SurfaceTests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is an intentional, deliberate breaking API change:
Surface(src/DemaConsulting.CanvasNet/Canvas/Surface.cs) now implementsIDisposable.Scope is deliberately narrow — API shape only, no pooling yet. This PR adds the
IDisposablecontract (a_disposedflag, an idempotent no-finalizerDispose(), andObjectDisposedException.ThrowIf(_disposed, this)guards on every public buffer-touching member) so thatSurface's internal pixel storage can be switched toArrayPool<byte>-backed rented arrays in a future, non-breaking PR. Plain-array-backed storage (new byte[...]) is completely unchanged in this PR; disposing aSurfacetoday does not release/return anything, it purely flips the guard flag. RealArrayPoolpooling is intentionally deferred to a dedicated follow-up PR (it has its own correctness subtleties — e.g.ArrayPool<T>.Rentreturning arrays larger than requested, and a security-relevant question about zeroing pixel data on return-to-pool — that deserve focused review, not to be bundled with this breaking API-shape change).Why now
Adding
IDisposabletoSurfaceis a binary/source breaking change (any code holding aSurfacein ausingblock today would newly be required to, though nothing is force-required by the compiler — see below). Making this change before 1.0 preserves the option ofArrayPool-based pooling post-1.0 without a second breaking change later.What changed
Surfaceis nowsealed class Surface : IDisposable.ObjectDisposedException.ThrowIf(_disposed, this)guards added to all 11 public members that touch the internal pixel buffer (13 call-sites, counting the indexer's get/set andCompositeOver(Surface)'s self/foreground checks separately): the indexer get/set,GetRowSpanBytes,GetRowSpan,Crop,PremultiplyAlpha,UnpremultiplyAlpha,Clear, bothCompositeOveroverloads, and both publicCompositeOverSpanoverloads. The twointernalworkspace-reusingCompositeOverSpanoverloads used byScanlineRasterizercarry the same guard (15 guards total), so a disposed surface cannot be mutated through the internal fill path either.GetRowSpanBytes/GetRowSpangained an explicit span-lifetime contract in the design doc: the disposal guard only runs when those methods are called, so a caller retaining a previously-obtained span across aDispose()call gets undefined behavior — callers must not do that.Dispose()is idempotent (safe to call twice) and has no finalizer — matching the exact precedent already established bySurface.cs's own internalRowChannelBuffers/CompositeWorkBuffershelper types, since these are managed arrays, not unmanaged resources.Canvas(src/DemaConsulting.CanvasNet/Rendering/Canvas.cs) does not becomeIDisposableand does not dispose theSurfaceit wraps — doc-only change adding explicit<remarks>to its constructor andSurfaceproperty stating this non-owning-reference contract (the caller that constructed theSurfaceremains responsible for disposing it).new Surface(...)call sites insrc//test/— verifiedCA2000is not a build-breaking diagnostic under this repo's analyzer settings, so none of them need to change to keep compiling/passing.README.mdanddocs/user_guide/introduction.mdcode samples that construct aSurfacenow use theusing var surface = new Surface(...)idiom, for consistency/teaching a good pattern.software-items.md: reqstream requirements (docs/reqstream/canvas-net/canvas.yaml,docs/reqstream/canvas-net/canvas/surface.yaml), design docs (docs/design/canvas-net/canvas/surface.md,docs/design/canvas-net/rendering/canvas.md), verification docs (docs/verification/canvas-net/canvas/surface.md,docs/verification/canvas-net/rendering/canvas.md), and the SysML2 model note (docs/sysml2/model/canvas-net/canvas/surface.sysml).New tests
Surface_Dispose_CalledOnce_DoesNotThrowSurface_Dispose_CalledTwice_DoesNotThrow(idempotency)*_AfterDispose_ThrowsObjectDisposedExceptiontest per guarded call-site, plus the foreground-disposed case forCompositeOver(Surface)(15 tests)Canvas_Surface_DisposedExternally_CanvasDoesNotThrowFromAccessingSurfaceProperty— a design-intent regression test documenting thatCanvascontinues to allow access to a disposedSurfacewithoutCanvasitself throwing.CanvasNet_SystemIntegration_DisposeSurfaceThenUseIt_ThrowsObjectDisposedException— a system-level integration test proving the disposal contract is reachable end to end through the public API, using a representative sample of guarded members (the full member-by-member matrix lives inSurfaceTests).Validation
pwsh ./fix.ps1run.TreatWarningsAsErrors=true).dotnet reqstream --lint,dotnet reviewmark --lint, SysML2 lint — all pass with 0 errors.Follow-up (tracked separately, not in this PR)
Surface's internal buffer (the internal implementation of the constructor +Dispose()), including the rented-array-size and clear-on-return design questions noted above.