Async version, remove depenency on RazorLight, implemented IDisposable

[mikaelliljedahl]

No description provided.

[mikaelliljedahl]

Finally got the tests to run. Had to do some adjustments to handle Async in the "Mockers" and update url to jkorpela.fi/cgi-bin/echo.cgi (missing https so we got redirected when submitting the forms).

Would be very glad if you could merge this PR and make a new nuget release (I already bumped the nuget version for it to be ready).

BR
Mikael

[takielias]

@kevingy Can You please check it?

[mikaelliljedahl]

@kevingy Can You please check it?

@takielias I created a forked branch here: https://github.com/mikaelliljedahl/SimpleHeadlessBrowser and nuget here: https://www.nuget.org/packages/SimpleHeadlessBrowser/
waiting for this PR to completed ;)

[kevingy]

Why? Browser does not have any unmanaged resources to require implementing IDisposable.

[mikaelliljedahl]

It helps the caller who forget to call Close to free up resources. It also fixes a memory leak if there is an Exception before the call to Close is done (or if the caller forgets). If not all frames are cleared there will be pointers to those hanging around. The Dispose method fixes those without the caller needing to know what is going on behind the hood.

[kevingy]

@mikaelliljedahl SimpleBrowser doesn't have any unmanaged resources, therefore IDisposable is not appropriate. In the case of an Exception, the clean up that is in Close occurs when the instance of Browser is garbage collected.

[mikaelliljedahl]

This was to solve this issue:
#121

Even if there are no unmanaged resources, if you have nested references inside a static variable that are never disposed the GC will never be able to do it's job. Since you have this static variable any references inside _allContext will prohibit the GC from freeing up resources.
This is inside Browser class.
private static readonly List<List> _allContexts = new List<List>();

The dispose method automatically calls the Close method even if exceptions happen. Maybe the Close method needs additional cleanup code to remove references from _allContexts. The important part is to make sure there are no references left from the static list to the disposed object (even if the user missed (eg due to exception) calling Close().

[mikaelliljedahl]

According to Microsoft docs:
There are additional reasons for implementing Dispose, such as undoing something that was previously done. For example, freeing memory that was allocated, removing an item from a collection that was added, signaling the release of a lock that was acquired, and so on

[kevingy]

In a PR to be implemented shortly, I'm going to fix the "memory leak" permanently by re-working the logging. The user will be able to configure how many pages they would like to navigate back - from 0 (no history) to the amount of memory they have. In my applications, I never use back. I'll probably set mine to 0.

[kevingy]

Why does the application need to sleep here?

[mikaelliljedahl]

Whoops, I used this in a project where the server wasn't fast enough. Should have put it in the program calling the code, not here. I am removing this.

[kevingy]

Please delete this commented code.

[mikaelliljedahl]

Done!

[kevingy]

I'm not sure I understand this solution. It looks like RazorLight has simply been replaced by RazorEngine. Was that the intention? I thought you wanted to be able to create a production library that didn't include a dependency on Razor.

[mikaelliljedahl]

The intention was to remove the dependency from the library, so I used a dependency injection pattern (using the IViewRenderService interface that only implements this method). In the sample program (which is not part of the nuget) I just show that RazorEngine could be used to render the log to string, but anything that implements the interface can be used. And if you like to use RazorLight, go ahead. The Interface doesn't care.

[kevingy]

I used this as a step to move forward. I removed the dependency on RazorLight from SimpleBrowser and put all of the rendering stuff into a separate library by implementing the interface and setting the renderer in SimpleBrowser. This way, any renderer can be implemented - razor, json, email, etc.

[takielias]

@mikaelliljedahl Can you please fix the above issues?

[mikaelliljedahl]

I have pushed the changes to the PreparePRToMainProject branch. Do I need to do something else to update the PR? (I am used to Microsoft DevOps)

[kevingy]

I have pushed the changes to the PreparePRToMainProject branch. Do I need to do something else to update the PR? (I am used to Microsoft DevOps)

I'll check, but that is all you should have to do.

[takielias]

@mikaelliljedahl Did you check all the comments of @kevingy ?

[mikaelliljedahl]

@mikaelliljedahl Did you check all the comments of @kevingy ?

Yea, but I guess we kind of disagree on the Browser object needed to implement IDisposable pattern. Another way to eliminate the memory leak problem is to completly remove the static variable containing the list of active browsers. That will of course change the behavior a bit. It has been done already in the separate fork branch that I created waiting for this PR to be accepted.

BR
Mikael

[takielias]

@mikaelliljedahl Did you check all the comments of @kevingy ?

Yea, but I guess we kind of disagree on the Browser object needed to implement IDisposable pattern. Another way to eliminate the memory leak problem is to completly remove the static variable containing the list of active browsers. That will of course change the behavior a bit. It has been done already in the separate fork branch that I created waiting for this PR to be accepted.

BR Mikael

@kevingy Could you please consider the PR?

[mikaelliljedahl]

@takielias You are free to use the fork, I made a fresh nuget for it too with the static list of browsers causing the memory leaks removed: https://www.nuget.org/packages/SimpleHeadlessBrowser/

[kevingy]

@mikaelliljedahl Implementing IDisposable will not fix the memory leak! You either don't have a full understanding of the problem internal to SimpleBrowser or you don't have a full understanding of IDisposable - or both.

SimpleBrowser maintains an infinitely growing collection of HTML nodes. So long as a Browser instance exists, the collection will grow every time a navigation occurs. (This is why I have always suggested transient Browser instances.) None of your changes address this real problem. Simply implementing IDisposable certainly won't fix it.

Furthermore, IDisposable "Provides a mechanism for releasing unmanaged resources." (https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-6.0). The collection contains all managed resources. In fact, I don't believe there are any unmanaged resources in SimpleBrowser. Everything is managed. Therefore, there is no need for IDisposable in the project at all.

There is good work in this PR. I wish you would have separated the work out so that I could have accepted parts of it at a time rather than a single huge PR. Additionally, your changes break backwards compatibility. I don't want to have to rewrite my SimpleBrowser applications to support your changes.

@takielias What is it in the PR that you need, exactly. Are you wanting the asynchronous methods? A "fix" for the memory leak?

[takielias]

@kevingy async is awesome but memory leak is a big issue.

[kevingy]

@kevingy async is awesome but memory leak is a big issue.

@takielias

Async is awesome! I would welcome it with open arms, so long as it was implemented in a way that didn't break backwards compatibility.

To avoid the memory leak, change your code to use transient Browser instances. That is, don't do this:

// This will leak, with an infinitely growing collection of HTML elements.
var browser = new Browser();
do 
{
    browser.Navigate("someurl");
} while (true);

Do this:

// This will still leak, but will be cleaned up when browser goes out of scope at the end of each loop.
do 
{
    var browser = new Browser();
    browser.Navigate("someurl");
} while (true);

The infinitely growing collection leak was added by, in my opinion, a bad contribution that was not fully vetted before being merged. To fix it, significant changes are required to the way that SimpleBrowser handles it's navigation history. The change was added so that when the user "clicks" the Back button, the SimpleBrowser loads the previous page from memory rather than navigating to the previous URL. So, the "leak" is an intentional "feature". The HTML element collection grows infinitely so that the user can click Back all the way to the first navigation. That feature (clicking the Back button) is probably little used and probably the greatest problem SimpleBrowser has.

[takielias]

@kevingy Thanks for the explanation. I used this Package in several projects but I did not need to use the Back button yet.

[mikaelliljedahl]

The "leak" you are describing is not what I would call a leak, it is just a consequence of how you use the browser if you open an infinite number of windows.
The problematic behavior is that the system can never free resources if there is an exception before calling Close();
The reason for this is that a static list of all browser windows are stored and those references are never removed unless you call Close().
IMO having a library that requires a consumer to call the Close method to remove the reference from the static list of all browsers is bad design. Having a static list of references to other browser windows stops the GC from freeing up memory causing the leak. The leak I am referring to is the one you have reproduced here:
#121
That is why I implemented an IDisposable pattern to make sure the cleanup is done automatically for you when your browser object goes out of scope (or due to an exception).

The implementation is not breaking the old behavior, you can just ignore the Disposable pattern and also ignore there is a Close method.
The Async pattern is neither a breaking change, you can still call the old non-async methods however you will get a build warning saying these methods are obsolete.

BR
Mikael

[kevingy]

This PR has been around forever. I'm going to start pulling pieces of it in at a time, then ultimately deny this pull request in favor of the individual pieces.

[mikaelliljedahl]

This PR has been around forever. I'm going to start pulling pieces of it in at a time, then ultimately deny this pull request in favor of the individual pieces.

How's it going pulling the pieces?