Bug 199568 - SystemViewRemoteFileAdapter#internalGetChildren() should not be synchronized since it could deadlock
Summary: SystemViewRemoteFileAdapter#internalGetChildren() should not be synchronized ...
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 2.0   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 2.0.1   Edit
Assignee: David McKnight CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-10 09:51 EDT by Martin Oberhuber CLA
Modified: 2007-09-11 12:28 EDT (History)
0 users

See Also:
mober.at+eclipse: review+


Attachments
patch to remove synchronized (1.68 KB, patch)
2007-09-11 12:04 EDT, David McKnight CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2007-08-10 09:51:49 EDT
SystemViewRemoteFileAdapter#internalGetChildren() is synchronized, but calls out to
other methods below it.

This construct could deadlock easily, since it is very similar to the one fixed
with bug #199552 -- see the comments there for information how the deadlock
actually happened.

Please investigate why you think the method must be synchronized, and either
get rid of the synchronized statement or find an alternative way of
synchronization. Take care of bug #149188 comment 4, it shows one reason why
the synchronized could be there.
Comment 1 Martin Oberhuber CLA 2007-08-10 09:52:22 EDT
For similar issues, see also bug 199565 and bug 199566.
Comment 2 Martin Oberhuber CLA 2007-08-10 10:16:46 EDT
See bug #199573 comment 1 for an example how to fix issues with synchronized.
Comment 3 Martin Oberhuber CLA 2007-09-10 08:42:00 EDT
Note that because there is only a single instance of the SystemViewRemoteFileAdapter, if ANY connection locks that one object through internalGetChildren(), NO other "remote files" connections will be able to get any children any more.

This is obvious from the backtrace in bug 202758 comment 1, where an FTP connection gets stalled because the remote server doesn't answer any more.

Due to the "synchronized" statements, no other connections will be able to get children; and, what's more, if any internalGetChildren() call should happen on a display thread, ALL of Eclipse will be stalled.

Please re-think the situation why you believe this should be synchronized and try to come up with a solution (perhaps eliminating the "synchronized" statement).
Comment 4 David McKnight CLA 2007-09-10 11:02:46 EDT
I tried taking out the synchronized for both SystemViewRemoteFileAdapter and I haven't been running into any concurrency issues.  I do however see a couple of additional calls to IFileService.getFile() on the main thread (by the table view when it tries to compute layout and by the system view when it's trying to determine matching filters).  I'm not sure if those can easily be avoided without risky changes right now.  At least the synchronized doesn't seem to be required.  Do you think it's safe enough to remove it at this stage? 
Comment 5 Martin Oberhuber CLA 2007-09-11 07:36:58 EDT
Take it out now, before the test pass this thursday. Even if we should run into trouble, the synchronized here is certainly not the right solution for it.
Comment 6 David McKnight CLA 2007-09-11 12:04:07 EDT
Created attachment 78073 [details]
patch to remove synchronized

Here is the patch to remove the synchronized from internalGetChildren
Comment 7 Martin Oberhuber CLA 2007-09-11 12:12:56 EDT
Patch is good, as discussed.
Comment 8 David McKnight CLA 2007-09-11 12:28:39 EDT
I've committed the patch for this to cvs.