Summary: | [rseterminal] Clean up RSE Terminal impl | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> | ||||||||
Component: | RSE | Assignee: | Anna Dushistova <anna.dushistova> | ||||||||
Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||||
Severity: | normal | ||||||||||
Priority: | P3 | CC: | dmcknigh, francescocriv, gpapayia, torkildr, wb-rel, xuanchen, ykuo | ||||||||
Version: | 3.0 | Keywords: | contributed, helpwanted | ||||||||
Target Milestone: | 3.0 M7 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Whiteboard: | |||||||||||
Bug Depends on: | 226550, 227572 | ||||||||||
Bug Blocks: | 231966 | ||||||||||
Attachments: |
|
Description
Martin Oberhuber
2008-04-23 21:37:10 EDT
I also notice the following: * Launch an SSH Terminal * Quit RSE * Restart * The terminal is not automatically re-created. * Remote Shell View does re-create shells on restart (if the corresponding Preference flag is set). Terminal should also be capable of restoring connections and terminals on quit and restart. Martin,
>Apart from those things, the patch seems to work fine. I'm missing a "Launch
>Terminal" on the files tree but that's a different separate bug, right?
for some reasons the part of our code that was doing it was not commited.
(Additions to testAttribute() in SystemViewRemoteFileAdapter).
But you anyway want us to get rid of *.files.* dependencies, right?
Yes, I got rid of that part of the patch because I didn't like the fact that whenever a new kind of shell was introduced some modification to the core RSE code would be required. The PropertyTester extension point allows contributing such checks by a 3rd party, and it provides better performance since the Eclipse Expression Language that's used in the ui.menus extensions can cache results. The corresponding bug that's asking to contribute the "Launch Shell" and "Launch Terminal" actions declaratively is in bug 226550. (In reply to comment #1) > * Remote Shell View does re-create shells on restart (if the corresponding > Preference flag is set). > > Terminal should also be capable of restoring connections and terminals on quit > and restart. Martin, could you provide me with some details? I wasn't able to find corresponding preference for Shells in Remote Systems preferences. Created attachment 98604 [details]
patch that fixes some issues mentioned
I, Anna Dushistova, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL. Patch looks good - 28 added and 40 removed lines - I'm applying it: [228577] [rseterminal] Clean up RSE Terminal impl (apply patch from Anna Dushistova) In TerminalServiceSubSystem, the implementation is not yet perfect: * ArrayList children should better be initialized right at the instance variable declaration, and not in separate constructors (--> less code, easier to understand) * In removeChild(Object) the if (size>0) is not needed because List.remove() is declared to be a valid no-op on an empty list --> less code * removeChild(String) as well as getChild(String) must be synchronized on the ArrayList OUTSIDE the iterator, because otherwise an exception could occur -- lists don't like getting changed by another Thread while an iterator is iterating over them (--> ConcurrentModificationException) * getChildren() and hasChildren() have unnecessary null checks and must likely be synchronized * cancelAllTerminals() has an unnecessary null check, and should first getChildren() and then iterate over the obtained array, in order to avoid an ArrayIndexOutOfBoundsException in case a Terminal is removed after obtaining the size -- correct operation is synchronized(children) { Object[] terminals = getChildren() children.clear(); } for(int i=terminals.length-1; i>=0; i--) { ... Can you please attach another patch fixing these issues. Created attachment 98633 [details]
patch to fix possible sync problems
Martin, take a look at this one please.
Created attachment 98685 [details]
Patch fixing more issues and adding Javadoc
Second patch is nice and fixes most issues, so I committed it.
But there was still more to clean up -- see attached patch (committed):
* Copyright Header did not include "All rights reserved." -- I think you should
add this to all your copyright headers.
* Removed "final" from TerminalServiceSubSystem -- I see no reason for this
to be final
* made _hostService private -- there are getters and setters available, no
need to have the field protected
* Removed inner class "Refresh" -- it made no sense having this public API,
a private fireAsyncRefresh() method accomplishes the same without exposing
unwanted API and is more efficient
* Added Javadoc Comments to all public or protected methods -- you should also
do that everywhere
* Removed protected constructor -- it was nowhere used and could not ever be
used since the class was declared final.
* Added if(elment!=null) around addChild() because we would have got problems
if a malicious client had added null elements -- also to be consistent with
removeChild()
* Using a synchronized iterator in getChild(String) is more efficient than
having to copy the entire collection into a temporary array. Note that I
also made TerminalElement.getName() final, in order to ensure that we don't
have an open call here.
* Simplified getChildren() and hasChildren() code
* Fixed potential threading issue in cancelAllTerminals(): Before my fix,
if a new terminal was added at almost the same time as running
cancelAllTerminals(), the List could have been cleared without properly
terminating the new terminal.
* In TerminalElement, fixed the equals() method -- before my fix, two
terminals with the same name but on different subsystems could have been
considered equal
Please review all fixes.
(In reply to comment #9) > > Please review all fixes. Martin, looks fine for me, thanks! BTW, do other subsystems like remote command one miss any synchronization on add/remove methods? Yes, it's easily possible that synchronization is missing in other parts of RSE. There hasn't been time to review all of RSE so far. But when we're adding something new, I like it done properly. Feel free to review other parts of RSE and suggest patches! For this bug, I also got rid of the terminals.ssh Activator. Remaining issues are now * package.html Javadoc -- please attach * Get rid of TerminalElement public API, unless you think it should be public API * For re-creating Terminals after quit & restart as per comment #1, I checked it and found that it also fails for remote shells - see bug 230285 - so let's leave this one out of the game for now. I'll mark this bug fixed when I have the package.html Javadoc. Not for M7 I'm marking this fixed in order to finish off our IP Log. Remaining issues (package.html javadoc) will be tracked in bug 231966. |