[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [dsdp-tm-dev] Re: DStoreConnectorService v1.57 is at risk of deadlock
|
Hi Martin,
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.
Any suggestions?
____________________________________
David McKnight
Phone: 905-413-3902 , T/L: 969-3902
Internet: dmcknigh@xxxxxxxxxx
Mail: D1/YFY/8200/TOR
____________________________________
David McKnight/Toronto/IBM@IBMCA
Sent by: dsdp-tm-dev-bounces@xxxxxxxxxxx
21/02/2008 08:04 AM
|
Please respond to
Target Management developer discussions <dsdp-tm-dev@xxxxxxxxxxx> |
|
|
To
| "Oberhuber, Martin" <Martin.Oberhuber@xxxxxxxxxxxxx>
|
|
cc
| Target Management developer discussions
<dsdp-tm-dev@xxxxxxxxxxx>
|
|
Subject
| [dsdp-tm-dev] Re: DStoreConnectorService
v1.57 is at risk of deadlock |
|
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
_______________________________________________
dsdp-tm-dev mailing list
dsdp-tm-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/dsdp-tm-dev
