Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic browser closure #2

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Automatic browser closure #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a5204b
Added Shutdown Hooks for Opened Browsers
MiriamKyoseva Jul 16, 2024
5a419bd
Update App.cs
MiriamKyoseva Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Bellatrix.Playwright/infrastructure/App.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using Bellatrix.Playwright.Controls.EventHandlers;
using Bellatrix.Playwright.Proxy;
using Bellatrix.Playwright.Services;
using Bellatrix.Playwright.Services.Browser;
using Bellatrix.Plugins;
using Bellatrix.Utilities;

Expand All @@ -35,6 +36,7 @@ public class App : IDisposable
public App()
{
_apiClientService = GetNewApiClientService();
AddShutdownHook();
}

public BrowserService Browser => ServicesCollection.Current.Resolve<BrowserService>();
Expand Down Expand Up @@ -159,4 +161,15 @@ public TPage GoTo<TPage>()
page.Open();
return page;
Comment on lines 161 to 162
Copy link

Choose a reason for hiding this comment

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

The method GoTo<TPage>() lacks error handling for scenarios where the page cannot be resolved or fails to open. This can lead to unhandled exceptions which may crash the application.

Recommended Solution:
Implement try-catch blocks around the page resolution and opening steps to handle potential exceptions gracefully and provide fallback or error logging mechanisms.

}

private void AddShutdownHook()
{
var container = ServicesCollection.Current;
var driver = container.Resolve<WrappedBrowser>();
AppDomain.CurrentDomain.ProcessExit += new EventHandler((sender, eventArgs) =>
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): The ProcessExit event handler is never unregistered, which could lead to memory leaks

Consider storing the EventHandler delegate as a field and removing it in the Dispose method using ProcessExit -=

{
DisposeBrowserService.Dispose(driver, container);
this.Dispose();
Comment on lines +171 to +172
Copy link

Choose a reason for hiding this comment

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

The DisposeBrowserService.Dispose(driver, container); call in the AddShutdownHook() method does not check if driver or container are already disposed or null. This could lead to unnecessary operations or errors during application shutdown.

Recommended Solution:
Add null checks and check if the resources are already disposed before attempting to dispose them again. This will prevent potential null reference exceptions and ensure cleaner resource management.

Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Calling Dispose in the shutdown hook could lead to double-dispose issues

Consider removing this call since the driver cleanup is already handled, or add a guard against multiple disposes

});
Comment on lines +169 to +173
Copy link

Choose a reason for hiding this comment

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

category Error Handling severity potentially major

Add error logging in the ProcessExit event handler.

Tell me more ​

Consider adding error logging in the ProcessExit event handler. If an exception occurs during the disposal process, it would be beneficial to log it for debugging purposes. You could wrap the disposal calls in a try-catch block and log any exceptions that occur.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

}
}
12 changes: 12 additions & 0 deletions src/Bellatrix.Web/infrastructure/App.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class App : IDisposable
public App()
{
_apiClientService = GetNewApiClientService();
AddShutdownHook();
}
Comment on lines +41 to 42
Copy link

Choose a reason for hiding this comment

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

Initializing resources and registering shutdown hooks directly in the constructor can lead to issues with testability and separation of concerns. Consider initializing critical resources like _apiClientService in a dedicated initialization method rather than the constructor. This approach enhances modularity and makes unit testing easier by allowing more control over the initialization process.

Recommended Change:

public App() { }

public void Initialize() {
    _apiClientService = GetNewApiClientService();
    AddShutdownHook();
}


public BrowserService Browser => ServicesCollection.Current.Resolve<BrowserService>();
Expand Down Expand Up @@ -216,4 +217,15 @@ private string DetermineTestClassFullNameAttributes()

return fullClassName;
}

private void AddShutdownHook()
Copy link

Choose a reason for hiding this comment

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

suggestion: Shutdown logic is duplicated across Playwright and Web implementations

Consider extracting the shutdown hook logic into a shared base class or utility to avoid duplication

public abstract class ShutdownHookBase
{
    protected void AddShutdownHook()
    {
        AppDomain.CurrentDomain.ProcessExit += OnProcessExit;
    }

    protected abstract void OnProcessExit(object sender, EventArgs e);
}

{
var container = ServicesCollection.Current;
var driver = container.Resolve<IWebDriver>();
AppDomain.CurrentDomain.ProcessExit += new EventHandler((sender, eventArgs) =>
{
DisposeDriverService.Dispose(driver, container);
this.Dispose();
});
Comment on lines +225 to +229
Copy link

Choose a reason for hiding this comment

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

Using AppDomain.CurrentDomain.ProcessExit for cleanup tasks is potentially unreliable for applications that can be terminated abruptly, as this event might not always fire. Additionally, resolving IWebDriver directly in the shutdown hook couples the cleanup logic tightly with the service's availability, which can lead to issues if the service is unavailable at shutdown time.

Recommended Change:
Consider using a more reliable mechanism for cleanup, such as implementing IDisposable and ensuring that all consumers of the class properly call Dispose. This change would decouple the shutdown process from the global state and improve the reliability of resource cleanup.

public void Dispose() {
    DisposeDriverService.Dispose(driver, ServicesCollection.Current);
    base.Dispose();
}

}
Comment on lines +221 to +230
Copy link

Choose a reason for hiding this comment

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

category Functionality severity potentially major

Resolve IWebDriver within the event handler in AddShutdownHook.

Tell me more ​

The current implementation of AddShutdownHook resolves the IWebDriver too early. Instead of resolving it when adding the hook, we should resolve it within the event handler to ensure we're disposing of the correct driver instance at shutdown. Consider modifying the method as follows:

private void AddShutdownHook()
{
    AppDomain.CurrentDomain.ProcessExit += new EventHandler((sender, eventArgs) =>
    {
        var container = ServicesCollection.Current;
        var driver = container.Resolve<IWebDriver>();
        DisposeDriverService.Dispose(driver, container);
        this.Dispose();
    });
}

This change will ensure that the correct IWebDriver instance is resolved and disposed of when the application is actually shutting down.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

}
Loading