[dsdp-tm-dev] Re: DStoreConnectorService v1.57 is at risk of deadlock

[David McKnight <dmcknigh@xxxxxxxxxx>]

Hi Martin,

The synchronized in DStoreConnectorService
was a mistake.  I had put it in earlier but forgot to remove it before
doing the message work.

For that string compare I'll look into
an alternative.  The original code was a comparision using message
ids which we no longer have.

For exc.printStackTrace(new
PrintWriter(excWriter)); I did put
a method in SimpleSystemMessage.  I guess I must have used that prior
to creating the throwsToDetails(Throwable) method.  I'll change that
code to SimpleSystemMessage for the exception.

I'll look for references to MessageFormat
(I think there may be others as well).

Yes, I agree that CANCELLED should be
changed to CANCELED.

I'll take a look at the clone() method.

I'll look at these other classes too.

Thanks,

____________________________________
David McKnight    
Phone:   905-413-3902 , T/L:  969-3902
Internet: dmcknigh@xxxxxxxxxx
Mail:       D1/YFY/8200/TOR
____________________________________

"Oberhuber, Martin" <Martin.Oberhuber@xxxxxxxxxxxxx>

21/02/2008 05:31 AM

To	David McKnight/Toronto/IBM@IBMCA
cc	"Target Management developer discussions" <dsdp-tm-dev@xxxxxxxxxxx>
Subject	DStoreConnectorService v1.57 is at risk of deadlock

Hi Dave,
 
your checkin of DStoreConnectorService v1.57
yesterday includes this:

protected synchronized void internalConnect(IProgressMonitor monitor) throws Exception

making this method synchronized is begging for deadlock. This is large method, with lots of calls outside ("open calls") including calls that do thread switches ("fireCommunicationsEvent"). Imagine this scenario:

• In the UI Thread, user presses a button to do dstore connect
• A background Job is scheduled to perform the connect
• synchronized internalConnect() is called, locking the DStoreConnectorService object for the background Thread
• asyncExec switches back to the UI thread
• In the UI thread, somebody needs something from DStoreConnectorService --> but the object is locked due to "synchronized" --> DEADLOCK.

Please get rid of
the "synchronized" statement again. 
And please remember to never ever put "synchronized"
on methods that have open calls which you cannot fully control.
 
Another problem is this:

if (msg != null && msg.getLevelOneText().startsWith(NLS.bind(ConnectorServiceResources.MSG_COMM_INVALID_LOGIN, getHostName())))

I think you should not compare NLS messages with startsWith() because this might fail in a BIDI environment. You simply don't have control of what language packs and NLS substitution does to your messages.

This code:

exc.printStackTrace(new PrintWriter(excWriter));

should be in a common place rather than ConnectionStatusListener -- what about having it in SimpleSystemMessage? Why wasn't this code needed before the Refactoring? You do have private static SimpleSystemMessage.throwableToDetails() don't you?

Next, in RexecDstoreServer you use MessageFormat.format() -- better use NLS.bind()

While at changing things, constants with CANCELLED should be renamed to CANCELED

Next, the implementation of SystemMessage#clone() is invalid (and has always been invalid). It needs to call super.clone() because the way you do it, if you would clone a SimpleSystemMessage you would come up with a SystemMessage (that's no longer simple). Please read the Javadocs of Object#clone().

Then, in the following classes the Message Strings need to be externalized: this was not possible before your change and thus was marked with //TODO dwd -- but now it should be done: RemoteFileCancelledException, RemoteFileIOException, RemoteFileSecurityException, RemoteFolderNotEmptyException

Messages with  "RSE","F","9999" should be migrated to your new SimpleSystemMessage, shouldn't they?

 
Thanks,
--
Martin Oberhuber, Senior Member of Technical Staff, Wind River
Target Management Project Lead, DSDP PMC Member
http://www.eclipse.org/dsdp/tm