Menu
▾
▴
#1259 Deadlock between main thread and JS executor
Hello again dear HtmlUnit maintainers,
I ran into the following problem while running the integration test suite for my application. The problem was intermittent and very difficult to track down. The symptoms of the problem were: elements missing from the page, the entire dom missing and the browser simply having the wrong page loaded in the window. So I cranked up the logging and was able to find the culprit. When the click is being process on the "main" thread after the actual response is downloaded it's added to the loadQueue_ in the WebClient to be processed later when the JavaScriptEngine.processPostponedActions method is invoked. This then invokes the JavaScriptEngine.doProcessPostponedActions and then the page is loaded into the target window in the WebClient.loadDownloadedResponses method. Now the JavaScriptEngine.doProcessPostponedActions is also invoked after scheduled java script jobs are executed. So what was happening in my application is that the click would add a load job on the "main" thread but that job would be picked off the queue by the "JS executor ..." thread. The WebClient.loadDownloadedResponse method will return if the loadQueue_ is empty so the "main" thread was returning before the page was actually loaded into the target window. The attached file contains a unit test that exposes the issue and the fix to the WebClient.loadDownloadedResponses implementation. I hope that I correctly understood the desired behavior for that method and that the fix I've provided is appropriate.
Thank you.
Fixed the loadDownloadedResponses method in the WebClient.
Thanks for the tiny test case. It's really great for such a problem.
Concerning the proposed fix: couldn't we remove synchronization on loadQueue_ if the whole block is protected with a ReentrantLock?
We can definitely protect access to the loadQueue_ with a reentrant lock. That would work exactly like the synchronized blocks in the current implementation. So where ever the loadQueue_ is accessed the lock would need to be acquired and then released and a try finally would need to wrap all access to the loadQueue_. To protect access to the loadQueue_ the synchronized blocks are more elegant since we don't need to sprinkle try finally all over the place, and we don't need the extra bells and whistles that are implemented by the reentrant lock. Additionally the current implementation allows for better parallelization one thread can be downloading responses while a different thread could be loading the already downloaded responses into windows. The reentrant lock is functioning as a mutex for the loadDownloadedResponses method, guaranteeing that only one thread at a time can execute it, if we remove the synchronized block guarding the loadQueue_ access then the loadQueue_ might get into an inconsistent state. This might simply be a problem with naming, if we simply rename the loadQueueMutex_ to loadDownloadedResponsesMutex_ then it will be clear what the purpose of the reentrant locks is. And since we don't need to use the advanced features of the ReentrantLock API it can be replaced with a simple object and a synchronized block which incapsulates the entire method body.
Is there anything else I can do to help get this patch integrated into the html unit code base?
Sorry for the delay. Now fixed in SVN.
I've incorporated your unit test in the build but for the fix I've found that the easiest way was to synchronize the method loadDownloadedResponses. If you see drawbacks to this solution, don't hesitate to add new comments.
Thanks for committing this change. The only difference between using a synchronized method and using a lock to accomplish the same functionality is that the synchronized method locks the instance of the object it's invoked on implicitly while using a separate lock makes it explicit and prevents anyone else from grabbing the lock that the method relies on without calling the method.
This change needs to be rolled back because it can lead to a deadlock.
Found one Java-level deadlock:
"JS executor for com.gargoylesoftware.htmlunit.WebClient@19f3146":
waiting to lock monitor 0000000036008ca0 (object 000000000e73d170, a com.gargoylesoftware.htmlunit.WebClient),
which is held by "main"
"main":
waiting to lock monitor 0000000036008d04 (object 000000000dde5268, a com.gargoylesoftware.htmlunit.html.HtmlPage),
which is held by "JS executor for com.gargoylesoftware.htmlunit.WebClient@19f3146"
Java stack information for the threads listed above:
"JS executor for com.gargoylesoftware.htmlunit.WebClient@19f3146":
at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2166)
"main":
at com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine$HtmlUnitContextAction.run(JavaScriptEngine.java:598)
- waiting to lock <000000000dde5268> (a com.gargoylesoftware.htmlunit.html.HtmlPage)
at net.sourceforge.htmlunit.corejs.javascript.Context.call(Context.java:537)
at net.sourceforge.htmlunit.corejs.javascript.ContextFactory.call(ContextFactory.java:538)
at com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine.callFunction(JavaScriptEngine.java:534)
at com.gargoylesoftware.htmlunit.html.HtmlPage.executeJavaScriptFunctionIfPossible(HtmlPage.java:897)
at com.gargoylesoftware.htmlunit.javascript.host.EventListenersContainer.executeEventListeners(EventListenersContainer.java:162)
at com.gargoylesoftware.htmlunit.javascript.host.EventListenersContainer.executeBubblingListeners(EventListenersContainer.java:221)
at com.gargoylesoftware.htmlunit.javascript.host.Node.fireEvent(Node.java:735)
at com.gargoylesoftware.htmlunit.html.HtmlElement$2.run(HtmlElement.java:866)
at net.sourceforge.htmlunit.corejs.javascript.Context.call(Context.java:537)
at net.sourceforge.htmlunit.corejs.javascript.ContextFactory.call(ContextFactory.java:538)
at com.gargoylesoftware.htmlunit.html.HtmlElement.fireEvent(HtmlElement.java:871)
at com.gargoylesoftware.htmlunit.html.HtmlPage.executeEventHandlersIfNeeded(HtmlPage.java:1166)
at com.gargoylesoftware.htmlunit.html.HtmlPage.initialize(HtmlPage.java:203)
at com.gargoylesoftware.htmlunit.WebClient.loadWebResponseInto(WebClient.java:450)
at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2201)
- locked <000000000e73d170> (a com.gargoylesoftware.htmlunit.WebClient)
at com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine.doProcessPostponedActions(JavaScriptEngine.java:634)
at com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine.processPostponedActions(JavaScriptEngine.java:716)
at com.gargoylesoftware.htmlunit.html.HtmlElement.click(HtmlElement.java:1246)
at com.gargoylesoftware.htmlunit.html.HtmlElement.click(HtmlElement.java:1195)
at com.gargoylesoftware.htmlunit.html.HtmlElement.click(HtmlElement.java:1158)
Reopening
Fixes loadDownloadedResponses without locking the webClient instance
I've uploaded another patch that fixes this issue without introducing new locks.
I don't like the usage of ThreadLocal for that. It may fix the problem in your case, but this is only the top of the iceberg. In fact the problem is that we still execute JS code from the main thread. This needs to be reworked to handle everything in the JS executor thread (except in GAE mode)
Renamed issue
Agreed. That would be a very big patch do you think it will be tackled before the 2.9 release?
This seems to still be an issue in 2.9.When do you think there will be a fix available?
Hi, I am using 2.9 with Java 7 and repeatedly facing this issue. Would there be a fix to this?
I was finding similar issues in 2.9, I've submitted a patch (now applied to trunk) under another issue to fix that problem - you may want to try latest trunk build to see if it fixes your issue?
Thanks a lot jp7621. I'll try it now and update this thread about how it went.
Hi,
I've been testing for last 2 days with the latest build and the deadlock hasn't happened yet. Looks like it is fixed. I'll update if I encoubter it again.
Here's another update. The deadlok issue is back, so it seems it wasn't completely resolved. However, When I run only one instance of Webclient, it doesn't seem to occur, but if I add multiple instance of WebClient on multiple user threads, the issue is back. Any ideas?
I really need to get this fixed as I plan to run my program over long periods without disruption. Any help is much appreciated.
De-assigning it from myself in case an other committer wants to work on this.
I've been experiencing something similar like this, when I run multiple threads of HTMLUnit each one continues to hang as time passes by. This even happens when I disable Javascript in HTMLUnit.
@Xpro: which HtmlUnit version are you using?
@Marc I experienced this with 2.10 and htmlunit-2.12-SNAPSHOT
Please be aware that modification of JavaScripEngine.doProcessPostponedActions to synchronized method can cause deadlock (commit from 15-10-2012: "fix some internal processing problem in our javascript engine; the old solution based on ThreadLocal was not correct because the method doProcessPosponedActions is called from different threads (from the main thread as part of the click() processing) and from the javascript thread after running some script").
In our application deadlock appears after clicking button (and I think it is some kind of race condition because sometimes deadlock doesn't appear - maybe some connection lags):
"State: BLOCKED on com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine@404ae411 owned by: Thread-1
(...)
com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine$HtmlUnitContextAction.run(JavaScriptEngine.java:651)
locked com.gargoylesoftware.htmlunit.html.HtmlPage@326cd1bd"
for Thread-1 thread:
"State: BLOCKED on com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine@404ae411 owned by: Thread-1
(...)
com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine$HtmlUnitContextAction.run(JavaScriptEngine.java:651)
Hope this helps resolving this issue.
Last edit: Adam Afeltowicz 2012-11-30
It seems for our case that removing "synchronized" from JavaScripEngine.doProcessPostponedActions did the trick, but it is hard for me to tell what is the impact of this change - can someone check it?