[RFC] [Plugin] Fixes 100% cpu load
Deepak Bhole
dbhole at redhat.com
Thu Sep 30 14:01:44 PDT 2010
Hi,
Sorry I couldn't get to this earlier.. wanted to study the patch
carefully before commenting.
* Andrew Su <asu at redhat.com> [2010-09-27 15:25]:
> 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.
>
I am a bit confused about this part. As it stands now, the sequence is
as follows:
init message comes in -> start init
destroy message comes in
if init is done -> destroy
if init is in progress -> wait, then destroy
Determination of if init is done or in progress is done via the status
hashmap which contains one of PRE_INIT, IN_INIT or INIT_COMPLETE.
Destroy cannot be handled before PRE_INIT is set, as PRE_INIT is the
first thing set, and this code will prevent destroy from proceeding
until it is:
// Wait till initialization finishes
while (!applets.containsKey(identifier) &&
(
!status.containsKey(identifier) ||
status.get(identifier).equals(PAV_INIT_STATUS.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.
>
I don't understand the destroy-before-init case. Are you seeing an instance
where destroy gets sent before init does? And if that's what happens, the while
loop above would still keep destroy from proceeding until it is initialized.
Is there a test case for this problem?
Sorry to be nitpicking.. I am trying to get rid of locks from the code as much
as possible by re-designing message sequencing... if a lock is to be added, I
want to make sure there is no other solution first :)
Cheers,
Deepak
> Cheers,
> Andrew
> 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) {
More information about the distro-pkg-dev
mailing list