Re: [dsdp-tm-dev] Re: DStoreConnectorService v1.57 is at risk of deadlock
I've dealt with most of this now. I
still have to deal with the startsWith() compare in DStoreConnectorService.
The issue that remains is what to do
about message IDs. In the original system message approach, the ID
was useful for determining where in the systemmessage.xml file to find
the corresponding message info. When using the SimpleSystemMessage
approach, the ID is not needed for that, however, it would still be useful
to be able to identify one message from another without comparing the strings.
One thing I like about SimpleSystemMessage is that the developer
isn't forced to come up with an ID but perhaps that's something we need.
Phone: 905-413-3902 , T/L: 969-3902
Sent by: dsdp-tm-dev-bounces@xxxxxxxxxxx
21/02/2008 08:04 AM
Please respond to
Target Management developer discussions <dsdp-tm-dev@xxxxxxxxxxx>
|"Oberhuber, Martin" <Martin.Oberhuber@xxxxxxxxxxxxx>
|Target Management developer discussions
|[dsdp-tm-dev] Re: DStoreConnectorService
v1.57 is at risk of deadlock|
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.
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
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.
Phone: 905-413-3902 , T/L: 969-3902
21/02/2008 05:31 AM
|"Target Management developer discussions"
|DStoreConnectorService v1.57 is at risk
your checkin of DStoreConnectorService v1.57 yesterday includes this:
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:
Please get rid of
the "synchronized" statement again.
- In the UI Thread, user presses a button to
do dstore connect
- A background Job is scheduled to perform
- 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"
And please remember to never ever put "synchronized" on methods
that have open calls which you cannot fully control.
Another problem is this:
(msg != null
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
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()
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
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?
Martin Oberhuber, Senior Member of Technical Staff, Wind
Target Management Project Lead, DSDP PMC Member
dsdp-tm-dev mailing list