Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Possibility to restart webkit server #443

Closed
wants to merge 1 commit into from

6 participants

@simonoff

No description provided.

@mhoran
Collaborator

Instead of killing the entire server, I'm more inclined to figure out why Session#reset! isn't cleaning up resources. Calling Session#reset! should delete all active WebPage instances, which in turn frees the NetworkAccessManager and all pending and finished QNetworkReply objects. If there's a leak somewhere, I'd rather clean that up, since killing the server is really just a cover up for a memory leak.

@simonoff

I'm not familiar in C++ so I cannot investigate why Session#reset! is not working. But memory leak is available when you using Webkit for scrapping instead testing purposes.

@mhoran
Collaborator

I'd recommend manually calling Session#reset! where you were planning to restart webkit_server. This should have the same effect, and is what normally happens between test runs with rspec and Cucumber.

@simonoff

Session#reset! not working! It not reset webkit browser. I have a some scrapper what uses capybara-webkit and after 2000 requests browser uses 2Gb of memory. Session#reset! don't reset browser and it still using 2Gb of memory. So for me is better kill webkit_server and run it again.

@seangeo

I've experienced similar behaviour on stillalive.com test runners. These were very long running processes that hit multiple websites running various scripts. I've found that restarting webkit_server between test runs gives the best overall stability. I have a hunch that webkit caching could be the culprit here but haven't investigated further, it's probably worth fixing though, because I think long running stability and webkit_server restarts are both valid use cases.

Having said that, this implementation should probably provide a destroy method on the browser or driver object, so you don't have to do session.driver.browser.connection.restart. In fact, the connection isn't publicly available on the browser object anyway... so unless I'm mistaken you'd have to hold onto a reference to the connection when the browser was created in order to reset it. So maybe the api needs some work here. I also think destroy is preferable to restart, since it makes sense to destroy and recreate the entire session object to avoid any possibility of left over state.

I have some mixins which I use internally to add the destroy behaviour to Browser which I could turn into a PR if you're interested.

@ezkl

@seangeo I'd be very interested in seeing the mixins you're using.

@jobseekerltd

There is a memory leak in WebPageManager.cpp:

WebPageManager::WebPageManager(QObject *parent) : QObject(parent) {
  m_ignoreSslErrors = false;
  m_cookieJar = new NetworkCookieJar(this);
  m_success = true;
  m_loggingEnabled = false;
  m_ignoredOutput = new QString();
  m_timeout = -1;
  createPage(this)->setFocus();
}

Debug messages will be appended m_ingoredOuput and it will keep growing infinitely as long as webkit server is running. I changed m_ignoredOutput to QFile and it reduces memory usage significantly for us:

  m_ignoredOutput = new QFile(this);

Unfortunately the our code lives in a private repo so I can't create a pull request. I hope someone can incorporate this into master branch.

@mhoran
Collaborator

Great find @climbingrose. I'll commit that change tomorrow.

@mhoran
Collaborator

I saw a 10MB decrease in memory usage while running the capybara-webkit tests with the change @climbingrose proposed. I've committed the change to master. It'd probably make sense not to instantiate a QDebug object every time the logger is called as well, but this is a great improvement already.

@zhengjia

With master branch I am still seeing increasing memory usage on webkit_server process. It grew about 300mb in about half an hour. I agree with @seangeo there should be a destroy method on the browser object. Selenium has a similar method browser.quit that can close and start a new browser window.

@mhoran
Collaborator

Closing this out for now. There's still no upstream API for this and I'd prefer to fix any memory leaks instead of implementing this as a workaround. If it really becomes an issue for folks I'm willing to reconsider. However even with large test suites on hosted CI instances I haven't run up against blocking memory issues.

@mhoran mhoran closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 17 additions and 1 deletion.
  1. +11 −1 lib/capybara/webkit/connection.rb
  2. +6 −0 spec/connection_spec.rb
View
12 lib/capybara/webkit/connection.rb
@@ -8,11 +8,15 @@ class Connection
SERVER_PATH = File.expand_path("../../../../bin/webkit_server", __FILE__)
WEBKIT_SERVER_START_TIMEOUT = 15
- attr_reader :port
+ attr_reader :port, :pid
def initialize(options = {})
@socket_class = options[:socket_class] || TCPSocket
@output_target = options.has_key?(:stdout) ? options[:stdout] : $stdout
+ start_server_and_connect
+ end
+
+ def start_server_and_connect
start_server
connect
end
@@ -33,6 +37,12 @@ def read(length)
@socket.read(length)
end
+ def restart
+ kill_process
+ @socket = nil
+ start_server_and_connect
+ end
+
private
def start_server
View
6 spec/connection_spec.rb
@@ -52,6 +52,12 @@
new_connection.port.should_not == connection.port
end
+ it 'restarts server' do
+ old_pid = connection.pid
+ connection.restart
+ connection.pid.should_not == old_pid
+ end
+
let(:connection) { Capybara::Webkit::Connection.new }
before(:all) do
Something went wrong with that request. Please try again.