skybrud / Skybrud.Umbraco.Redirects

Redirects manager for Umbraco.
https://packages.limbo.works/skybrud.umbraco.redirects/
MIT License
38 stars 43 forks source link

Memory leak - Context is never disposed? #198

Closed JasonElkin closed 8 months ago

JasonElkin commented 8 months ago

Which version of Skybrud Redirects are you using? (Please write the exact version, example: 4.0.8)

4.0.22

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

12.3.6

Bug description

When a 404 is hit, and there is no Umbraco Context, a new one gets created - it looks like this instance is never actually disposed of.

https://github.com/skybrud/Skybrud.Umbraco.Redirects/blob/3464b26ede760edeb4bd8e3d4c3b91ce8b0a8815/src/Skybrud.Umbraco.Redirects/Middleware/RedirectsMiddleware.cs#L61

I'm not 100% on this, but maybe because the reference is not used in the scope of the method it can't be automatically disposed of?

abjerner commented 8 months ago

Hi @JasonElkin and thanks for reporting this 👍

Do you have any evidence to suggest that this does in fact create a memory leak?

Even though reference isn't used, I would assume it's disposed once outside of the switch case, our at least the method. But I will try to do some testing to verify this 😉

JasonElkin commented 8 months ago

Not to worry, I've tested it properly now @abjerner and this isn't the culprit - the context reference is disposed of as expected.

abjerner commented 8 months ago

@JasonElkin better safe than sorry 😉

I had to head out before I could post my findings here, but I also came to the same conclusion. I did some testing by setting up my own instance of IUmbracoContextFactory and UmbracoContextReference so I could log when the UmbracoContextReference was disposed.

The redirects middleware uses the IUmbracoContextFactory because an IUmbracoContext isn't available for requests for static files, but even testing with these, the UmbracoContextReference would be correctly disposed.

JasonElkin commented 8 months ago

Yeah, that's exactly what I did @abjerner

Only shame is I now need to go hunting again for the source of this pesky leak 🕵️‍♂️