Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #227572 +++ From bug 227572 comment 8: What I'd like to see now is get rid of unnecessary public API from subsystems.terminals.core and add Javadoc for the remaining public API classes as well as the API package.html. In TerminalServiceSubSystem#addChild() and removeChild() it looks to me like some "synchronized" is missing. Right now, when two threads call the remove() method at almost the same time, it can happen that the communications listener is added twice; and in the removeChild() method, it looks like the communications listener cannot ever be removed because children is never going to be null. I also don't understand why adding the comm listener is necessary here, since it's already done in the initializeSubSystem() / uninitializeSubSystem() methods. In general, I cannot see why you instantiate the "children" list lazily. Code gets much simpler if you just create an empty ArrayList in the constructor, and then test for children.size()==0. cancelAllTerminals() has a e.printStackTrace(); which I don't like -- better log any errors to Eclipse rather than printing to stdout. RSETerminalConnectionThread also has one that should be removed. subsystems.terminals.ssh could also live without an Activator, I'd think, since there is no dynamic activation required. In TerminalViewer, you set a context help ID ("ucmd0000") but where is the associated context help in the user docs? 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?
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.
