Fixed Issue#1628 #1629

br3aker · 2021-05-14T03:11:49Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)
This PR does not break existing public API in any way

Description

Fixes #1628
Some public properties can't break anything (e.g image size or metadata), they didn't throw before this PR, this behaviour wasn't altered. ToString weren't affected either.

Due to slight internal (within Image class) API change pixel indexer might be a bit more performant: EnsureNotDisposed() call is no longer virtual. Although JIT in some runtimes even could inline such virtual calls in previous versions, it's still a nice change of having one less virtual method I guess.
This change also affects any public method but their calls are too rare to affect performance.

…r being disposed

…- it is done at public method calls

… was disposed, check is delegated to public methods using that property

…having a Finalization method

internal call EnsureNotDisposed is no longer virtual -> micro speedup gain in pixel index accessor Image<TPixel>[x, y].

As Image does not have unmanaged resources and does not implement finalizer method, there's no need for disposable pattern with a pair of Dispose() & Dispose(bool). Due Dispose(bool) was changed to DisposeManaged().

Subject to discuss. Image public properties Height, Width, Metadata and PixelType can't corrupt anything if backing image was disposed so I don't see any point altering that behaviour, it wasn't throwing before this branch and shouldn't throw after.

…ensures all operations are called on valid object

… if object was disposed

CLAassistant · 2021-05-14T16:15:09Z

All committers have signed the CLA.

codecov · 2021-05-15T13:35:53Z

Codecov Report

Merging #1629 (5d2884e) into master (39697f4) will increase coverage by 0.02%.
The diff coverage is 97.56%.

❗ Current head 5d2884e differs from pull request most recent head afee881. Consider uploading reports for the commit afee881 to get more accurate results

@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
+ Coverage   84.12%   84.15%   +0.02%     
==========================================
  Files         812      812              
  Lines       35592    35634      +42     
  Branches     4146     4147       +1     
==========================================
+ Hits        29943    29987      +44     
  Misses       4831     4831              
+ Partials      818      816       -2

Flag	Coverage Δ
unittests	`84.15% <97.56%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp/ImageFrameCollection{TPixel}.cs	`91.40% <92.00%> (+2.12%)`	⬆️
src/ImageSharp/Image.cs	`95.12% <100.00%> (+1.00%)`	⬆️
src/ImageSharp/ImageFrameCollection.cs	`96.55% <100.00%> (+6.55%)`	⬆️
src/ImageSharp/Image{TPixel}.cs	`93.90% <100.00%> (+1.21%)`	⬆️

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 39697f4...afee881. Read the comment docs.

JimBobSquarePants · 2021-05-18T11:31:46Z

I'll get this reviewed ASAP. Thanks for the contribution!

JimBobSquarePants

Definitely the right direction but we need to fix up the Disposal pattern changes to match convention and prevent breaking the API.

JimBobSquarePants · 2021-05-21T15:50:18Z

src/ImageSharp/Image.cs

        /// </summary>
-        /// <param name="disposing">Whether to dispose of managed and unmanaged objects.</param>
-        protected abstract void Dispose(bool disposing);
+        protected abstract void DisposeManaged();


This breaks common convention and the API so I can't allow it.

Providing an abstract protected abstract void Dispose(bool disposing) is well documented and should be the pattern we use.

https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#dispose-and-disposebool

src/ImageSharp/Image{TPixel}.cs

JimBobSquarePants · 2021-05-21T15:57:07Z

tests/ImageSharp.Tests/Image/ImageTests.cs

+            public void Save_ObjectDisposedException()
+            {
+                var image = new Image<Rgba32>(this.configuration, 10, 10);
+                var stream = new MemoryStream();


using stream. The fact that the GC cleans up here is an implementation detail.

JimBobSquarePants · 2021-05-21T16:03:38Z

src/ImageSharp/Image.cs

+        {
+            if (this.isDisposed)
+            {
+                throw new ObjectDisposedException("Trying to execute an operation on a disposed image.");


We should be using a ThrowHelper here to allow inlining on sealed instances.

You mean ThrowHelper class from ThrowHelper.cs? Should I add ThrowArgumentException method or am I missing something? (there is none as far as I can see :P)

JimBobSquarePants · 2021-05-21T16:07:10Z

src/ImageSharp/ImageFrameCollection.cs

@@ -11,8 +11,10 @@ namespace SixLabors.ImageSharp
    /// Encapsulates a pixel-agnostic collection of <see cref="ImageFrame"/> instances
    /// that make up an <see cref="Image"/>.
    /// </summary>
-    public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
+    public abstract class ImageFrameCollection : IDisposable, IEnumerable<ImageFrame>


Wow. I hadn't realized this wasn't already implementing the type. Good catch!

JimBobSquarePants · 2021-05-21T16:11:44Z

src/ImageSharp/Image.cs

@@ -80,8 +82,14 @@ protected Image(Configuration configuration, PixelTypeInfo pixelType, ImageMetad
        /// <inheritdoc />
        public void Dispose()
        {
-            this.Dispose(true);
-            GC.SuppressFinalize(this);


We shouldn't be removing the finalizer suppression in the base class.

https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#the-dispose-method

Didn't know SuppressFinalize do nothing if type has no finalizer, thanks!

JimBobSquarePants · 2021-05-21T16:15:05Z

src/ImageSharp/Image{TPixel}.cs

-                throw new ObjectDisposedException("Trying to execute an operation on a disposed image.");
-            }
-        }
+        protected override void DisposeManaged() => this.frames.Dispose();


The original pattern was correct here.

…d raw throws with this in Image/ImageFrameCollection

br3aker · 2021-05-21T19:14:52Z

@JimBobSquarePants seems like I've messed up with ThrowHelper from shared infrastructure. Although I'd added it locally, I've totally missed that it wasn't in the commit and it isn't tracked by git now. How to fix it?
First time working with such a huge project, sorry for being such a burden.

JimBobSquarePants · 2021-05-22T13:35:02Z

First time working with such a huge project, sorry for being such a burden.

Not at all!! You're contributions are most welcome!

That's my mistake for not being clear. ThrowHelper in the ShareInfrastructure project cannot be edited (As it's included via a submodule) but it is a partial so I would make another part for that class in the ImageSharp project and add your method there.

br3aker · 2021-05-22T13:58:15Z

Got it, thanks for answering yet another question!

JimBobSquarePants · 2021-06-02T14:53:03Z

@br3aker Can you please give me write access to your fork. For some reason I'm unable to push directly to your PR so we can complete this.

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

br3aker · 2021-06-02T15:20:41Z

@JimBobSquarePants that's strange. Edit was enabled from the start, I re-checked it now, can you try it again please?

JimBobSquarePants · 2021-06-02T15:35:10Z

@br3aker This isn't the first PR I've had issues with and I'm not sure why it happens. I tried again and it didn't work. The last time the contributor had to explicitly invite me as a contributor to the repo as a workaround.

br3aker · 2021-06-02T15:50:59Z

@JimBobSquarePants invite sent, thanks for fixing this!

JimBobSquarePants

LGTM 👍 Thanks!

Dmitry Pentin added 15 commits May 13, 2021 22:19

Image<TPixel>.Frames now properly throws ObjectDisposedException afte…

7900b43

…r being disposed

Image<TPixel> private methods no longer check if object was disposed …

d48b152

…- it is done at public method calls

Image<TPixel> private property PixelSource no longer checks if object…

7029b2f

… was disposed, check is delegated to public methods using that property

Removed GC.SuppressFinalize(this) from Image.Dispose() due to it not …

1c45c1a

…having a Finalization method

Added tests for issue#1628

095ce41

Moved dispose control logic to base Image class

acf9d85

internal call EnsureNotDisposed is no longer virtual -> micro speedup gain in pixel index accessor Image<TPixel>[x, y].

Removed redundant flag from Image.Dispose(bool) call

8ec1013

As Image does not have unmanaged resources and does not implement finalizer method, there's no need for disposable pattern with a pair of Dispose() & Dispose(bool). Due Dispose(bool) was changed to DisposeManaged().

Removed invalid tests

ff4b269

Subject to discuss. Image public properties Height, Width, Metadata and PixelType can't corrupt anything if backing image was disposed so I don't see any point altering that behaviour, it wasn't throwing before this branch and shouldn't throw after.

ImageFrameCollection now properly implements IDisposable interface & …

cbca565

…ensures all operations are called on valid object

All ImageFrameCollection<TPixel> public properties & method now check…

127e9dd

… if object was disposed

Added internal accessor for root frame

3f8bd3d

Added tests for issues#1628

009e935

Fixed couple of invalid tests for ImageFrameCollection<TPixel>

f0f0c08

ImageFrameCollection.Contains first checks if it was disposed first

a71ce19

Fixed a couple of failing tests

f561053

JimBobSquarePants added API codequality labels May 18, 2021

Merge branch 'master' into image-disposed-fix

34706fb

JimBobSquarePants requested changes May 21, 2021

View reviewed changes

Dmitry Pentin added 4 commits May 21, 2021 20:16

Renamed private Image<TPixel>.PixelSourse to PixelSourceUnsafe

1c8dcef

Implemented dispose method according to common convention.

e787ffa

Fixed disposable resouce leak in unit test.

d54ff0e

Implemented ThrowObjectDisposedException for the ThrowHelper, replace…

5704403

…d raw throws with this in Image/ImageFrameCollection

Fix throwhelper

65808ae

Merge branch 'master' into pr/1629

5d2884e

JimBobSquarePants approved these changes Jun 2, 2021

View reviewed changes

Make frames resonly

afee881

JimBobSquarePants merged commit 3764b1c into SixLabors:master Jun 2, 2021

JimBobSquarePants deleted the image-disposed-fix branch June 2, 2021 16:29

renovate bot mentioned this pull request Mar 7, 2022

Update dependency SixLabors.ImageSharp to v2 - autoclosed RononDex/Astrobot#78

Closed

1 task

renovate bot mentioned this pull request Mar 26, 2022

chore(deps): update dependency sixlabors.imagesharp to v2 FantasticFiasco/export-image-dotnet-core#32

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Issue#1628 #1629

Fixed Issue#1628 #1629

br3aker commented May 14, 2021 •

edited

Loading

CLAassistant commented May 14, 2021 •

edited

Loading

codecov bot commented May 15, 2021 •

edited

Loading

JimBobSquarePants commented May 18, 2021

JimBobSquarePants left a comment

JimBobSquarePants May 21, 2021

JimBobSquarePants May 21, 2021

JimBobSquarePants May 21, 2021

br3aker May 21, 2021

JimBobSquarePants May 21, 2021

JimBobSquarePants May 21, 2021

br3aker May 21, 2021

JimBobSquarePants May 21, 2021

br3aker commented May 21, 2021 •

edited

Loading

JimBobSquarePants commented May 22, 2021

br3aker commented May 22, 2021

JimBobSquarePants commented Jun 2, 2021 •

edited

Loading

br3aker commented Jun 2, 2021 •

edited

Loading

JimBobSquarePants commented Jun 2, 2021

br3aker commented Jun 2, 2021

JimBobSquarePants left a comment

Fixed Issue#1628 #1629

Fixed Issue#1628 #1629

Conversation

br3aker commented May 14, 2021 • edited Loading

Prerequisites

Description

CLAassistant commented May 14, 2021 • edited Loading

codecov bot commented May 15, 2021 • edited Loading

Codecov Report

JimBobSquarePants commented May 18, 2021

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br3aker commented May 21, 2021 • edited Loading

JimBobSquarePants commented May 22, 2021

br3aker commented May 22, 2021

JimBobSquarePants commented Jun 2, 2021 • edited Loading

br3aker commented Jun 2, 2021 • edited Loading

JimBobSquarePants commented Jun 2, 2021

br3aker commented Jun 2, 2021

JimBobSquarePants left a comment

Choose a reason for hiding this comment

br3aker commented May 14, 2021 •

edited

Loading

CLAassistant commented May 14, 2021 •

edited

Loading

codecov bot commented May 15, 2021 •

edited

Loading

br3aker commented May 21, 2021 •

edited

Loading

JimBobSquarePants commented Jun 2, 2021 •

edited

Loading

br3aker commented Jun 2, 2021 •

edited

Loading