[RFC] [Plugin] Fixes 100% cpu load
Dr Andrew John Hughes
ahughes at redhat.com
Tue Sep 28 10:17:04 PDT 2010
On 15:25 Mon 27 Sep , Andrew Su wrote:
> Hello,
>
> This patch fixes the following:
> *Plugin goes into a deadlock waiting for another thread to initialize applet, and no more threads are available to consume messages.
> *Plugin handles "destroy" message when another thread trying to create the applet is kicked off processor before setting the status to PRE_INIT.
>
>
> Example.
> We have 2 MessageConsumer threads.
> Passes "destroy" message to both of them for applet 1 and applet 2.
> Thread 1: Spinlock waiting for another thread to initialize applet 1.
> Thread 2: Spinlock waiting for another thread to initialize applet 2.
> Both threads are now in deadlock.
>
> Proposed Patch:
> When message is destroy, don't spinlock, handle it right away.
> -Problem with this: handles destroy right before we start initializing the applet.
> -Fix to new problem: Synchronize the two blocks
> --Case 1:If in process of initializing, finish initializing. handle the destroy.
> --Case 2:If destroy already handled, don't initialize.
>
>
> Cheers,
> Andrew
Some points:
* Why is the access to appLock protected by mainLock in the first block
but the later get() is not?
* On the same subject, why is appLock a Hashtable rather than a HashMap? Do you
need the synchronisation this provides? If so, would it not be better to synchronise
a HashMap or use one of the concurrent collections with better performance? It's unclear
whether the Hashtable's internal lock is protecting its contents, whether mainLock is
or some strange combination of both.
* The second synchronised block is far too big. Is this really necessary?
* Seems to be using 8 space indentation, making this hard to read.
It would help a lot if you could explain how this solution is supposed to work,
and how you intend appLock to function. To that end, this should be documented in
the code too.
> diff -r 5c0d756b4bb6 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Fri Sep 24 15:25:51 2010 +0100
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon Sep 27 14:59:43 2010 -0400
> @@ -360,6 +360,9 @@
>
> private static Long requestIdentityCounter = 0L;
>
> + private static Hashtable<Integer,boolean[]> appLock = new Hashtable<Integer, boolean[]>();
> + private static int[] mainLock = {0};
> +
> /**
> * Null constructor to allow instantiation via newInstance()
> */
> @@ -505,78 +508,92 @@
> */
> public static void handleMessage(int identifier, int reference, String message)
> {
> -
> + synchronized(mainLock){
> + if (!appLock.containsKey(identifier)){
> + boolean[] lock = {false};
> + appLock.put(identifier, lock);
> + }
> + }
> +
> PluginDebug.debug("PAV handling: " + message);
>
> try {
> if (message.startsWith("handle")) {
> -
> - // Extract the information from the message
> - String[] msgParts = new String[4];
> - for (int i=0; i < 3; i++) {
> - int spaceLocation = message.indexOf(' ');
> - int nextSpaceLocation = message.indexOf(' ', spaceLocation+1);
> - msgParts[i] = message.substring(spaceLocation + 1, nextSpaceLocation);
> - message = message.substring(nextSpaceLocation + 1);
> + synchronized(appLock.get(identifier)){
> + if (!status.containsKey(identifier)){
> + // Extract the information from the message
> + String[] msgParts = new String[4];
> + for (int i=0; i < 3; i++) {
> + int spaceLocation = message.indexOf(' ');
> + int nextSpaceLocation = message.indexOf(' ', spaceLocation+1);
> + msgParts[i] = message.substring(spaceLocation + 1, nextSpaceLocation);
> + message = message.substring(nextSpaceLocation + 1);
> + }
> +
> + long handle = Long.parseLong(msgParts[0]);
> + String width = msgParts[1];
> + String height = msgParts[2];
> +
> + int spaceLocation = message.indexOf(' ', "tag".length()+1);
> + String documentBase =
> + UrlUtil.decode(message.substring("tag".length() + 1, spaceLocation));
> + String tag = message.substring(spaceLocation+1);
> +
> + PluginDebug.debug ("Handle = " + handle + "\n" +
> + "Width = " + width + "\n" +
> + "Height = " + height + "\n" +
> + "DocumentBase = " + documentBase + "\n" +
> + "Tag = " + tag);
> +
> + status.put(identifier, PAV_INIT_STATUS.PRE_INIT);
> + PluginAppletViewer.parse
> + (identifier, handle, width, height,
> + new StringReader(tag),
> + new URL(documentBase));
> +
> +
> + int maxWait = APPLET_TIMEOUT; // wait for applet to fully load
> + int wait = 0;
> + while (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> + (wait < maxWait)) {
> +
> + try {
> + Thread.sleep(50);
> + wait += 50;
> + } catch (InterruptedException ie) {
> + // just wait
> + }
> + }
> +
> + if (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE))
> + throw new Exception("Applet initialization timeout");
> +
> + PluginAppletViewer oldFrame = applets.get(identifier);
> + reFrame(oldFrame, oldFrame.identifier, oldFrame.statusMsgStream,
> + handle, oldFrame.panel);
> + }
> + appLock.get(identifier).notifyAll();
> }
> -
> - long handle = Long.parseLong(msgParts[0]);
> - String width = msgParts[1];
> - String height = msgParts[2];
> -
> - int spaceLocation = message.indexOf(' ', "tag".length()+1);
> - String documentBase =
> - UrlUtil.decode(message.substring("tag".length() + 1, spaceLocation));
> - String tag = message.substring(spaceLocation+1);
> -
> - PluginDebug.debug ("Handle = " + handle + "\n" +
> - "Width = " + width + "\n" +
> - "Height = " + height + "\n" +
> - "DocumentBase = " + documentBase + "\n" +
> - "Tag = " + tag);
> -
> - status.put(identifier, PAV_INIT_STATUS.PRE_INIT);
> - PluginAppletViewer.parse
> - (identifier, handle, width, height,
> - new StringReader(tag),
> - new URL(documentBase));
> -
> -
> - int maxWait = APPLET_TIMEOUT; // wait for applet to fully load
> - int wait = 0;
> - while (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> - (wait < maxWait)) {
> -
> - try {
> - Thread.sleep(50);
> - wait += 50;
> - } catch (InterruptedException ie) {
> - // just wait
> - }
> - }
> -
> - if (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE))
> - throw new Exception("Applet initialization timeout");
> -
> - PluginAppletViewer oldFrame = applets.get(identifier);
> - reFrame(oldFrame, oldFrame.identifier, oldFrame.statusMsgStream,
> - handle, oldFrame.panel);
> -
> } else {
> - PluginDebug.debug ("Handling message: " + message + " instance " + identifier + " " + Thread.currentThread());
> -
> - // Wait till initialization finishes
> - while (!applets.containsKey(identifier) &&
> - (
> - !status.containsKey(identifier) ||
> - status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT)
> - )
> - );
> -
> - // don't bother processing further for inactive applets
> - if (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE))
> - return;
> -
> + synchronized(appLock.get(identifier)){
> + PluginDebug.debug ("Handling message: " + message + " instance " + identifier + " " + Thread.currentThread());
> +
> + // Wait till initialization finishes
> + while (!applets.containsKey(identifier) &&
> + (
> + !status.containsKey(identifier) ||
> + status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT)
> + ) && !message.startsWith("destroy")
> + ){ appLock.get(identifier).wait(5*1000000000);}
> +
> + if (message.startsWith("destroy"))
> + applets.get(identifier).handleMessage(reference, message);
> +
> + // don't bother processing further for inactive applets
> + if (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE))
> + return;
> + }
> +
> applets.get(identifier).handleMessage(reference, message);
> }
> } catch (Exception e) {
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net
PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint = F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
More information about the distro-pkg-dev
mailing list