Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
[dsdp-tm-dev] 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
 
 

Back to the top