-
-
Notifications
You must be signed in to change notification settings - Fork 30
Simplify browser management with single thread-safe instance #63
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.
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,23 +13,39 @@ module FerrumPdf | |
| autoload :Controller, "ferrum_pdf/controller" | ||
| autoload :HTMLPreprocessor, "ferrum_pdf/html_preprocessor" | ||
|
|
||
| mattr_accessor :browser_mutex, default: Mutex.new | ||
| mattr_accessor :include_assets_helper_module, default: true | ||
| mattr_accessor :include_controller_module, default: true | ||
| mattr_accessor :browsers, default: {} | ||
| mattr_accessor :config, default: ActiveSupport::OrderedOptions.new.merge( | ||
| window_size: [ 1920, 1080 ] | ||
| ) | ||
|
|
||
| # This doesn't use mattr_accessor because having a `.browser` getter and also | ||
| # having local variables named `browser` would be confusing. For simplicity, | ||
| # this is explicitly accessed. | ||
| @@browser = nil | ||
|
|
||
| class << self | ||
| def configure | ||
| yield config | ||
| end | ||
|
|
||
| # Creates and registers a new browser for exports | ||
| # If a browser with the same name already exists, it will be shut down before instantiating the new one | ||
| def add_browser(name, **options) | ||
| @@browsers[name].quit if @@browsers[name] | ||
| @@browsers[name] = Ferrum::Browser.new(@@config.merge(options)) | ||
| # Sets the browser instance to use for all operations | ||
| # If a browser is already set, it will be shut down before setting the new one | ||
| def browser=(browser_instance) | ||
| browser_mutex.synchronize do | ||
| @@browser&.quit | ||
| @@browser = browser_instance | ||
| end | ||
| end | ||
|
|
||
| # Provides thread-safe access to the browser instance | ||
| def with_browser | ||
| browser_mutex.synchronize do | ||
| @@browser ||= Ferrum::Browser.new(config) | ||
| @@browser.restart unless @@browser.client.present? | ||
| yield @@browser | ||
| end | ||
| end | ||
|
|
||
| # Renders HTML or URL to PDF | ||
|
|
@@ -68,36 +84,32 @@ def render_screenshot(screenshot_options: {}, **load_page_args) | |
| # | ||
| # This automatically applies HTML preprocessing if `html:` is present | ||
| # | ||
| def load_page(url: nil, html: nil, base_url: nil, authorize: nil, wait_for_idle_options: nil, browser: :default, retries: 1) | ||
| def load_page(url: nil, html: nil, base_url: nil, authorize: nil, wait_for_idle_options: nil, retries: 1) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should keep the That wouldn't need to quit the existing browser like reassigning the default browser would. |
||
| try = 0 | ||
|
|
||
| # Lookup browser if a name was passed | ||
| browser = @@browsers[browser] || add_browser(browser) if browser.is_a? Symbol | ||
| with_browser do |browser| | ||
| # Closes page automatically after block finishes | ||
| # https://github.com/rubycdp/ferrum/blob/main/lib/ferrum/browser.rb#L169 | ||
| browser.create_page do |page| | ||
| page.network.authorize(**authorize) { |req| req.continue } if authorize | ||
|
|
||
| # Automatically restart the browser if it was disconnected | ||
| browser.restart unless browser.client.present? | ||
| # Load content | ||
| if html | ||
| page.content = FerrumPdf::HTMLPreprocessor.process(html, base_url) | ||
| else | ||
| page.go_to(url) | ||
| end | ||
|
|
||
| # Closes page automatically after block finishes | ||
| # https://github.com/rubycdp/ferrum/blob/main/lib/ferrum/browser.rb#L169 | ||
| browser.create_page do |page| | ||
| page.network.authorize(**authorize) { |req| req.continue } if authorize | ||
| # Wait for everything to load | ||
| page.network.wait_for_idle(**wait_for_idle_options) | ||
|
|
||
| # Load content | ||
| if html | ||
| page.content = FerrumPdf::HTMLPreprocessor.process(html, base_url) | ||
| else | ||
| page.go_to(url) | ||
| yield browser, page | ||
| end | ||
|
|
||
| # Wait for everything to load | ||
| page.network.wait_for_idle(**wait_for_idle_options) | ||
|
|
||
| yield browser, page | ||
| end | ||
| rescue Ferrum::DeadBrowserError | ||
| try += 1 | ||
| if try <= retries | ||
| browser.restart | ||
| with_browser(&:restart) | ||
| retry | ||
| else | ||
| raise | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,67 @@ | ||
| require "test_helper" | ||
|
|
||
| class FerrumPdfTest < ActiveSupport::TestCase | ||
| def teardown | ||
| # Reset browser state after each test to avoid flaky behavior | ||
| FerrumPdf.browser = nil | ||
| end | ||
|
|
||
| test "it has a version number" do | ||
| assert FerrumPdf::VERSION | ||
| end | ||
|
|
||
| test "re-registering browser shuts down previous browser" do | ||
| first_browser = FerrumPdf.add_browser :example | ||
| test "setting new browser replaces previous browser" do | ||
| first_browser = Ferrum::Browser.new | ||
| first_pid = first_browser.process.pid | ||
| FerrumPdf.browser = first_browser | ||
|
|
||
| second_browser = FerrumPdf.add_browser :example | ||
| second_browser = Ferrum::Browser.new | ||
| second_pid = second_browser.process.pid | ||
| FerrumPdf.browser = second_browser | ||
|
|
||
| # First browser should be shut down | ||
| assert_nil first_browser.process | ||
|
|
||
| # Second browser should have a different Process ID | ||
| assert_not_equal first_pid, second_pid | ||
|
|
||
| FerrumPdf.with_browser do |yielded_browser| | ||
| assert_same second_browser, yielded_browser | ||
| end | ||
| end | ||
|
|
||
| test "auto-creates browser when none is set" do | ||
| FerrumPdf.browser = nil | ||
|
|
||
| FerrumPdf.with_browser do |browser| | ||
| assert_instance_of Ferrum::Browser, browser | ||
| assert browser.client.present? | ||
| end | ||
| end | ||
|
|
||
| test "auto-created browser uses config settings" do | ||
| original_config = FerrumPdf.config.dup | ||
| FerrumPdf.config.window_size = [ 800, 600 ] | ||
|
|
||
| FerrumPdf.browser = nil | ||
|
|
||
| FerrumPdf.with_browser do |browser| | ||
| assert_instance_of Ferrum::Browser, browser | ||
| assert_equal [ 800, 600 ], browser.options.window_size | ||
| end | ||
| ensure | ||
| FerrumPdf.config.replace(original_config) | ||
| end | ||
|
|
||
| test "reuses same browser instance across multiple with_browser calls" do | ||
| FerrumPdf.browser = nil | ||
|
|
||
| first_call_browser = nil | ||
| second_call_browser = nil | ||
|
|
||
| FerrumPdf.with_browser { |browser| first_call_browser = browser } | ||
| FerrumPdf.with_browser { |browser| second_call_browser = browser } | ||
|
|
||
| assert_same first_call_browser, second_call_browser | ||
| end | ||
| end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to support the
browser: niloption forrender_pdfto let users pass in their own separate browser, we could do something like this.