nddipiazza commented 6 years ago

tl;dr

DO NOT MERGE this still needs work.

In com.machinepublishers.jbrowserdriver.JBrowserDriverServer#get in the finally block where it is going to wait for a status code, the AppThread.exec method needs to use some sort of timeout. Otherwise it will wait upwards of 10000ms in the sample program below, even though the user is expecting a 2000ms timeout.

In this PR, I use the pageLoadTimeoutMS as a timeout parameter call which is better than nothing.

To verify

import com.google.common.base.Stopwatch;
import com.machinepublishers.jbrowserdriver.Settings;
import com.machinepublishers.jbrowserdriver.JBrowserDriver;
import com.sun.net.httpserver.HttpExchange;
import com.sun.net.httpserver.HttpHandler;
import com.sun.net.httpserver.HttpServer;

import java.io.IOException;
import java.io.OutputStream;
import java.net.InetSocketAddress;
import java.util.concurrent.TimeUnit;

class TestDriverOpts {
  static class MyHandler implements HttpHandler {
    @Override
    public void handle(HttpExchange t) throws IOException {
      String response = "<html>\n" +
          "  <head>\n" +
          "    <script>\n" +
          "      while (true) { }\n" +
          "    </script>\n" +
          "  </head>\n" +
          "</html>";
      t.sendResponseHeaders(200, response.length());
      OutputStream os = t.getResponseBody();
      os.write(response.getBytes());
      os.close();
    }
  }

  public static void main(String[] args) throws InterruptedException, IOException {
    HttpServer server = HttpServer.create(new InetSocketAddress(8000), 0);
    server.createContext("/index.html", new MyHandler());
    server.start();
    Settings.Builder builder = Settings.builder()
        .quickRender(true)
        .ajaxResourceTimeout(2000)
        .ajaxWait(10)
        .connectionReqTimeout(2000)
        .connectTimeout(2000);
//        .javaOptions("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005");
    JBrowserDriver driver = new JBrowserDriver(builder.build());
    driver.manage().timeouts().setScriptTimeout(2000, TimeUnit.MILLISECONDS);
    driver.manage().timeouts().pageLoadTimeout(2000, TimeUnit.MILLISECONDS);

    for (int i = 0; i < 100; i++) {
      Thread.sleep(1000L);
      Stopwatch stopwatch = Stopwatch.createStarted();
      try {
        System.out.println("BEGIN");
        driver.get("http://127.0.0.1:8000/index.html");
        String result = driver.getPageSource();
        System.out.println(result);
        System.out.println("END took " + stopwatch.elapsed(TimeUnit.MILLISECONDS) + " ms");
      } catch (Exception e) {
         System.out.println("ERROR: " + e.getMessage() + " took " + stopwatch.elapsed(TimeUnit.MILLISECONDS));
      }
    }
    Thread.sleep(500000);
    System.exit(0);
  }
}

Expected: Timeout should not be 10000ms, it should be closer to the page timeout.

NOTE: This still needs work. In this state it is waiting 2*pageTimeout

If I replace

https://github.com/MachinePublishers/jBrowserDriver/pull/291/files#diff-048530290a10b616323dcd32f8c164e1R340

with

}, 500);

it will be much closer to the actual timeout. What would be the side effect of doing this?

hollingsworthd commented 6 years ago

Thanks for this. I think instead of following the page load timeout that page cancellations should either have their own configurable timeout, use one of the script-related timeouts, or just use some value probably good enough such as 3 seconds.

I'd just want to make sure that when the timeout is reached that JavaFX actually cancels the event and it doesn't stick around taking up resources. If it still isn't really canceled then this whole approach might need some rethinking.

Again, thanks for this and the associated bug report #292.

nddipiazza commented 6 years ago

@hollingsworthd any word on this fix? I am probably going to use my PR here in my next release unless you are going to post a PR of this soon.

hollingsworthd-alt commented 6 years ago

@nddipiazza sorry I've been indisposed. Not able to merge PR's etc at the moment. I've been interested in getting more contributors access to merge PR's and make releases. Once I'm able to would like to add you as contributor but currently I'm not sure when I can do that.

hollingsworthd commented 6 years ago

Looks good. I think this is the appropriate way to fix this. Thanks very much for contributing this and for your patience waiting!

hollingsworthd commented 6 years ago

This is released as v0.17.11 available via Maven Central

MachinePublishers / jBrowserDriver

JBrowserDriverServer.get 's finally block needs a timeout #291

tl;dr

To verify