From ea5a616466f95586f06da3c3b29ace81afe5a23c Mon Sep 17 00:00:00 2001 From: Andy Grunwald Date: Sun, 8 Sep 2024 20:41:54 +0200 Subject: [PATCH] Quit Chrome instance gracefully, if an abort signal is sent Fix #30 --- cmd/sync.go | 83 ++++++++++++++++++++++++++---------------- lib/browser/browser.go | 44 +++++++++++----------- lib/browser/oauth2.go | 45 ++++++++++++----------- lib/browser/quit.go | 15 ++++++++ 4 files changed, 114 insertions(+), 73 deletions(-) create mode 100644 lib/browser/quit.go diff --git a/cmd/sync.go b/cmd/sync.go index ea70be6..572e5c4 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "log/slog" "strings" @@ -249,30 +250,46 @@ func runRecipes(p *tea.Program, logger *slog.Logger, supplier, localOICDBChecksu logger.Info("Downloading invoices ...", "supplier", recipesToExecute[i].recipe.Supplier, "supplier_type", recipesToExecute[i].recipe.Type) switch recipesToExecute[i].recipe.Type { case "browser": - browserDriver := browser.NewBrowserDriver(logger, recipeCredentials, buchhalterDocumentsDirectory, documentArchive, buchhalterMaxDownloadFilesPerReceipt) + browserDriver, err := browser.NewBrowserDriver(logger, recipeCredentials, buchhalterDocumentsDirectory, documentArchive, buchhalterMaxDownloadFilesPerReceipt) + if err != nil { + // TODO Implement better error handling + fmt.Println(err) + } + + // Send the browser context to the view layer + // This is needed in case of an external abort signal (e.g. CTRL+C). + p.Send(updateBrowserContext{ctx: browserDriver.GetContext()}) + recipeResult = browserDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) if ChromeVersion == "" { ChromeVersion = browserDriver.ChromeVersion } - // TODO Should we quit it here or inside RunRecipe? - err = browserDriver.Quit() + + // We don't need to call `chromedp.Cancel()` here. + // The browserDriver will be closed gracefully when the recipe is finished. + // In case of an external abort signal (e.g. CTRL+C), bubbletea will call `chromedp.Cancel()`. + + case "client": + clientDriver, err := browser.NewClientAuthBrowserDriver(logger, recipeCredentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory, documentArchive) if err != nil { // TODO Implement better error handling fmt.Println(err) } - case "client": - clientDriver := browser.NewClientAuthBrowserDriver(logger, recipeCredentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory, documentArchive) + + // Send the browser context to the view layer + // This is needed in case of an external abort signal (e.g. CTRL+C). + p.Send(updateBrowserContext{ctx: clientDriver.GetContext()}) + recipeResult = clientDriver.RunRecipe(p, totalStepCount, stepCountInCurrentRecipe, baseCountStep, recipesToExecute[i].recipe) if ChromeVersion == "" { ChromeVersion = clientDriver.ChromeVersion } - // TODO Should we quit it here or inside RunRecipe? - err = clientDriver.Quit() - if err != nil { - // TODO Implement better error handling - fmt.Println(err) - } + + // We don't need to call `chromedp.Cancel()` here. + // The browserDriver will be closed gracefully when the recipe is finished. + // In case of an external abort signal (e.g. CTRL+C), bubbletea will call `chromedp.Cancel()`. } + rdx := repository.RunDataSupplier{ Supplier: recipesToExecute[i].recipe.Supplier, Version: recipesToExecute[i].recipe.Version, @@ -473,6 +490,14 @@ type viewModel struct { buchhalterAPIClient *repository.BuchhalterAPIClient recipeParser *parser.RecipeParser logger *slog.Logger + + // Browser + browserCtx context.Context +} + +// updateBrowserContext is a message type to update the browser context in the bubbletea application. +type updateBrowserContext struct { + ctx context.Context } // viewMsgQuit initiates the shutdown sequence for the bubbletea application. @@ -550,6 +575,9 @@ func initialModel(logger *slog.Logger, vaultProvider *vault.Provider1Password, b buchhalterAPIClient: buchhalterAPIClient, recipeParser: recipeParser, logger: logger, + + // Browser + browserCtx: nil, } return m @@ -611,6 +639,11 @@ func (m viewModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil + case updateBrowserContext: + m.logger.Info("Updating browser context") + m.browserCtx = msg.ctx + return m, nil + case viewMsgQuit: m.logger.Info("Initiating shutdown sequence") @@ -761,26 +794,14 @@ func quit(m viewModel) viewModel { m.details = "HAVE A NICE DAY! :)" } - // TODO Double check where we need to quit running browser sessions - // TODO Wait group for browser and client - /* - go func() { - err := browser.Quit() - if err != nil { - // TODO implement better error handling - fmt.Println(err) - } - }() - */ - /* - go func() { - err := client.Quit() - if err != nil { - // TODO implement better error handling - fmt.Println(err) - } - }() - */ + // Stopping the browser instance + m.logger.Info("Stopping browser instance") + if m.browserCtx != nil { + err := browser.Quit(m.browserCtx) + if err != nil { + m.logger.Error("Error cancelling browser", "error", err) + } + } return m } diff --git a/lib/browser/browser.go b/lib/browser/browser.go index df66796..d29faee 100644 --- a/lib/browser/browser.go +++ b/lib/browser/browser.go @@ -44,6 +44,7 @@ type BrowserDriver struct { documentsDirectory string browserCtx context.Context + browserCancel context.CancelFunc recipeTimeout time.Duration maxFilesDownloaded int @@ -55,24 +56,20 @@ type BrowserDriver struct { newFilesCount int } -func NewBrowserDriver(logger *slog.Logger, credentials *vault.Credentials, buchhalterDocumentsDirectory string, documentArchive *archive.DocumentArchive, maxFilesDownloaded int) *BrowserDriver { - return &BrowserDriver{ +func NewBrowserDriver(logger *slog.Logger, credentials *vault.Credentials, buchhalterDocumentsDirectory string, documentArchive *archive.DocumentArchive, maxFilesDownloaded int) (*BrowserDriver, error) { + driver := &BrowserDriver{ logger: logger, credentials: credentials, documentArchive: documentArchive, buchhalterDocumentsDirectory: buchhalterDocumentsDirectory, - browserCtx: context.Background(), + browserCtx: nil, + browserCancel: nil, recipeTimeout: 60 * time.Second, maxFilesDownloaded: maxFilesDownloaded, newFilesCount: 0, } -} - -func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) utils.RecipeResult { - // Init browser - b.logger.Info("Starting chrome browser driver ...", "recipe", recipe.Supplier, "recipe_version", recipe.Version) // Setting chrome flags // Docs: https://github.com/GoogleChrome/chrome-launcher/blob/main/docs/chrome-flags-for-tools.md @@ -82,17 +79,29 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI chromedp.Flag("headless", false), ) - ctx, cancel, err := cu.New(cu.NewConfig( - cu.WithContext(b.browserCtx), + var err error + driver.browserCtx, driver.browserCancel, err = cu.New(cu.NewConfig( + cu.WithContext(context.Background()), cu.WithChromeFlags(opts...), // create a timeout as a safety net to prevent any infinite wait loops cu.WithTimeout(600*time.Second), )) if err != nil { - // TODO Implement error handling - panic(err) + return driver, err } - defer cancel() + + return driver, nil +} + +func (b *BrowserDriver) GetContext() context.Context { + return b.browserCtx +} + +func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) utils.RecipeResult { + b.logger.Info("Starting chrome browser driver ...", "recipe", recipe.Supplier, "recipe_version", recipe.Version) + + ctx := b.browserCtx + defer b.browserCancel() // get chrome version for metrics if b.ChromeVersion == "" { @@ -109,6 +118,7 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI b.logger.Info("Starting chrome browser driver ... completed ", "recipe", recipe.Supplier, "recipe_version", recipe.Version, "chrome_version", b.ChromeVersion) // create download directories + var err error b.downloadsDirectory, b.documentsDirectory, err = utils.InitSupplierDirectories(b.buchhalterDocumentsDirectory, recipe.Supplier) if err != nil { // TODO Implement error handling @@ -267,14 +277,6 @@ func (b *BrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountI return result } -func (b *BrowserDriver) Quit() error { - if b.browserCtx != nil { - return chromedp.Cancel(b.browserCtx) - } - - return nil -} - func (b *BrowserDriver) stepOpen(ctx context.Context, step parser.Step) utils.StepResult { b.logger.Debug("Executing recipe step", "action", step.Action, "url", step.URL) diff --git a/lib/browser/oauth2.go b/lib/browser/oauth2.go index bdaa1aa..9feb6c6 100644 --- a/lib/browser/oauth2.go +++ b/lib/browser/oauth2.go @@ -47,8 +47,9 @@ type ClientAuthBrowserDriver struct { downloadsDirectory string documentsDirectory string - recipeTimeout time.Duration browserCtx context.Context + browserCancel context.CancelFunc + recipeTimeout time.Duration newFilesCount int oauth2AuthToken string @@ -61,8 +62,8 @@ type ClientAuthBrowserDriver struct { oauth2PkceVerifierLength int } -func NewClientAuthBrowserDriver(logger *slog.Logger, credentials *vault.Credentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory string, documentArchive *archive.DocumentArchive) *ClientAuthBrowserDriver { - return &ClientAuthBrowserDriver{ +func NewClientAuthBrowserDriver(logger *slog.Logger, credentials *vault.Credentials, buchhalterConfigDirectory, buchhalterDocumentsDirectory string, documentArchive *archive.DocumentArchive) (*ClientAuthBrowserDriver, error) { + driver := &ClientAuthBrowserDriver{ logger: logger, credentials: credentials, documentArchive: documentArchive, @@ -70,14 +71,11 @@ func NewClientAuthBrowserDriver(logger *slog.Logger, credentials *vault.Credenti buchhalterConfigDirectory: buchhalterConfigDirectory, buchhalterDocumentsDirectory: buchhalterDocumentsDirectory, + browserCtx: nil, + browserCancel: nil, recipeTimeout: 120 * time.Second, - browserCtx: context.Background(), newFilesCount: 0, } -} - -func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) utils.RecipeResult { - b.logger.Info("Starting client auth chrome browser driver ...", "recipe", recipe.Supplier, "recipe_version", recipe.Version) // Setting chrome flags // Docs: https://github.com/GoogleChrome/chrome-launcher/blob/main/docs/chrome-flags-for-tools.md @@ -87,17 +85,29 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, chromedp.Flag("headless", false), ) - ctx, cancel, err := cu.New(cu.NewConfig( - cu.WithContext(b.browserCtx), + var err error + driver.browserCtx, driver.browserCancel, err = cu.New(cu.NewConfig( + cu.WithContext(context.Background()), cu.WithChromeFlags(opts...), // create a timeout as a safety net to prevent any infinite wait loops cu.WithTimeout(600*time.Second), )) if err != nil { - // TODO Implement error handling - panic(err) + return driver, err } - defer cancel() + + return driver, nil +} + +func (b *ClientAuthBrowserDriver) GetContext() context.Context { + return b.browserCtx +} + +func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, stepCountInCurrentRecipe int, baseCountStep int, recipe *parser.Recipe) utils.RecipeResult { + b.logger.Info("Starting client auth chrome browser driver ...", "recipe", recipe.Supplier, "recipe_version", recipe.Version) + + ctx := b.browserCtx + defer b.browserCancel() // get chrome version for metrics if b.ChromeVersion == "" { @@ -114,6 +124,7 @@ func (b *ClientAuthBrowserDriver) RunRecipe(p *tea.Program, totalStepCount int, b.logger.Info("Starting client auth chrome browser driver ... completed ", "recipe", recipe.Supplier, "recipe_version", recipe.Version, "chrome_version", b.ChromeVersion) // create download directories + var err error b.downloadsDirectory, b.documentsDirectory, err = utils.InitSupplierDirectories(b.buchhalterDocumentsDirectory, recipe.Supplier) if err != nil { // TODO Implement error handling @@ -603,11 +614,3 @@ func extractJsonRecursive(data interface{}, keys []string) []string { return results } - -func (b *ClientAuthBrowserDriver) Quit() error { - if b.browserCtx != nil { - return chromedp.Cancel(b.browserCtx) - } - - return nil -} diff --git a/lib/browser/quit.go b/lib/browser/quit.go new file mode 100644 index 0000000..ee6485e --- /dev/null +++ b/lib/browser/quit.go @@ -0,0 +1,15 @@ +package browser + +import ( + "context" + + "github.com/chromedp/chromedp" +) + +// Quit cancels a chromedp context, waits for its resources to be cleaned up, +// and returns any error encountered during that process. +// +// ctx needs to be a chromedp.Context to work properly. +func Quit(ctx context.Context) error { + return chromedp.Cancel(ctx) +}