Bug 4575 - desire exception checks for inputProc/windowCallback (1G55O7P)
Summary: desire exception checks for inputProc/windowCallback (1G55O7P)
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 2.0   Edit
Hardware: All Neutrino
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Silenio Quarti CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2001-10-11 14:19 EDT by Mike Wilson CLA
Modified: 2004-02-27 12:12 EST (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Wilson CLA 2001-10-11 14:19:14 EDT
PJM (11/28/00 10:31:57 PM)
	When in applet mode, both the window callbacks and input proc callbacks 
are being called into from
	(potentially), the PtMainLoop() that the plugin is running.  ie, there 
is no Java code calling these
	methods, or any C code that we control at all.  This causes a problem 
when an exception is
	thrown, as it goes right by all the methods in the frame, expecting 
someone down low to
	handle it.  Noone there to do it.  

	I would like to see the following sort of code added for both the 
inputProc and windowCallback,
	so that I can at least diagnose exceptions.  Currently, I have to sort 
of guess that  	an exception occurred and that's why my syncExec() never 
returns".  I can stick a breakpoint in
	Throwable's constructor and catch it that way


int windowProc (int handle, int data, int info) throws Throwable {
	try {
		Widget widget = WidgetTable.get (handle);
		if (widget == null) return OS.Pt_CONTINUE;
		return widget.processEvent (data, info);
	}
	catch (Throwable e) {
		if (embedded) e.printStackTrace();
		throw e;
	}
}

	Also, I had patched runAsyncMessages() before, as when the lock threw 
an exception,
	it never bothered to wake up the sender.  I think it is perfectly valid 
to do this.  ie, 
	change the code as follows:

boolean runAsyncMessages () {
	if (messagesSize == 0) return false;
	do {
		RunnableLock lock = removeFirst ();
		if (lock == null) return true;
		synchronized (lock) {
			try {							
			// ---- added this line
				lock.run ();
			}							
				// ---- added this line
			finally {						
		// ---- added this line
				lock.notifyAll ();
			}							
				// ---- added this line
		};
	} while (true);
}

	I don't know, maybe not.  I guess at this point the readAndDispatch() 
thread would be
	dead.  However, in the applet case, I think everything would be fairly 
clean.  Might get
	some mouse problems if exceptions did get thrown, but it would be worth 
it.

NOTES:

PJM (12/7/00 12:28:23 AM)
	I'd really like to get the try{} finally{} in runAsyncMessages, as I 
just noticed a thread
	blocked on a Runnable that was long gone, and it was tying up my AWT 
Event Queue.
	Here are my current versions:

int inputProc (int data, int rcvid, int message, int size) {
	if (!embedded) return OS.Pt_CONTINUE;

	try {	
		runDeferredEvents ();
		runAsyncMessages ();
	}
	catch (Throwable e) {
		e.printStackTrace();
	}
	
	return OS.Pt_CONTINUE;
}

boolean runAsyncMessages () {
	if (messagesSize == 0) return false;
	do {
		RunnableLock lock = removeFirst ();
		if (lock == null) return true;
		synchronized (lock) {
			try {
				lock.run ();
			}
			finally {
				lock.notifyAll ();
			}
		};
	} while (true);
}

int windowProc (int handle, int data, int info) {
	Widget widget = WidgetTable.get (handle);
	if (widget == null) return OS.Pt_CONTINUE;
	
	if (!embedded) return widget.processEvent (data, info);
	
	try {
		return widget.processEvent (data, info);
	}
	catch (Throwable e) {
		e.printStackTrace();
		return OS.Pt_CONTINUE;
	}
	
}

SSQ/SN (12/7/00 4:30:00 PM) -
	We don't think adding debug statement in the code is right. We plan to 
do
	something like the following:

1) Add this field to com.ibm.swt.widgets.RunnableLock

	Throwable throwable;

2) Change these methods in com.ibm.swt.widgets.Display

public void syncExec (Runnable runnable) {
	if (isValidThread ()) {
		if (runnable != null) runnable.run ();
		return;
	}
	if (runnable == null) {
		wake ();
		return;
	}
	RunnableLock lock = new RunnableLock (runnable);
	synchronized (lock) {
		addLast (lock);
		wake ();
		boolean interrupted = false;
		while (!lock.done ()) {
			try {
				lock.wait ();
			} catch (InterruptedException e) {
				interrupted = true;
			}
		}
		if (interrupted) {
			Thread.currentThread ().interrupt ();
		}
************ ADDED  ************
		if (lock.throwable != null) {
			SWTError.error(SWT.ERROR_UNSPECIFIED, lock.throwable);
		}
***************************************
	}
}

boolean runAsyncMessages () {
	if (messagesSize == 0) return false;
	do {
		RunnableLock lock = removeFirst ();
		if (lock == null) return true;
		synchronized (lock) {
*************** ADDED ***************
			try {
***************************************
				lock.run ();
*************** ADDED ***************
			} catch (Throwable t) {
				lock.throwable = t;
			}
***************************************
			lock.notifyAll ();
		};
	} while (true);
}

	So that the caller of Display.syncExec() will be notified that an 
exception ocurred while
	running the syncExec runnable.

PJM (12/11/00 4:57:50 PM)
	This is going to have a new side effect of having exceptions 
transported from the UI thread as
	a rando exception, into the thread doing the syncExec() as an SWTError.

	I guess this basically solves my issue of ensuring the lock.notifyAll() 
is always run, which it
	seems like it would in this case.  It doesn't help at all with my 
desire to have debugging code
	in windowProc or inputProc though, unless I missed something.

	Given that, I don't think having a possible SWTError out of syncExec 
really helps >me<.  I
	use it all over the place, and clearly can't add execption handlers to 
it.  I can't imagine
	too many people doing this.  Besides, you will get different behaviour 
if an exception
	is thrown in your syncExec() depending if you are on the UI thread or 
not (wrapped in an
	SWTError if you aren't on UI thread, not wrapped in an SWTError if you 
are on a UI 
	thread).

	Given all this, it seems safest to just change the lock.run(); call into
		try {
			lock.run();
		}
		finally {
			lock.notifyAll();
		}

	The debugging statements in windowProc and inputProc, per my 
suggestion, only
	get used for the embedded=true case.  I don't see much harm in using 
them as is.

SSQ (3/16/01 3:55:26 PM) -
	We don't believe that just notifing the caller thread is the right 
thing to do,
	since this hides the fact that the runnable failed to run completly and 
can lead to
	things being null and other unexpected conditions in the caller code.

PJM (8/31/2001 12:02:21 PM)
	Have some code that runs in a syncExec() 
	under Voyager, and if it throws an exception, we lock up the entire 
applet
	system.

	Again, reason for this is basically that the lock is not sent notifyAll
(), so the
	thread that did the syncExec() will never wake up again.  Locked 
forever.

	Plus, because we have no Java code or C code we control that is 
basically
	calling the callin points of inputProc() et al, we actually have no 
idea that
	an exception has actually occurred.

	So, I've gone back and had to patch in this same friggin code again to
	get the customer limping.  Code added is the code to catch throwable 
and 
	print the stack trace if (embedded == true), and the movement of the
	lock.notifyAll() code to inside the finally {} block instead of outside 
of it.
	

SSQ (9/6/01 2:39:22 PM) -
	The consensus is that we will do 2 things:
		- put a "debug flag" into SWT to optionally allow stack traces 
to be printed.
		- cause some kind of SWTError to occur on the thread which 
called the syncExec, 
		rather than leave it hanging forever.

	Need to decide whatr is going to happen with the UI thread. Should we 
hide the exception
	from the UI thread, or not?
Comment 1 DJ Houghton CLA 2001-10-29 16:22:54 EST
PRODUCT VERSION:

	SWT for Photon 0.004

Comment 2 Silenio Quarti CLA 2004-02-27 12:12:52 EST
Working on this is fixed. I throw SWT.ERROR_FAILED_EXEC and the debug flag is added.