Community
Participate
Working Groups
Build ID: TM 3.3m4 on Eclipse 3.6.1, on RHEL4 (szg-qa-lx1) Likely also an issue with TM 3.2.1 To reproduce: Create an RSE "SSH Only" connection. Connect "My Home". Launch a Terminal from RSE. In the terminal, type "exit". Right-click disconnect. Quit Eclipse. On the stdout console where Eclipse had been launched, I see this: looking for type: got text/plain Job found still running after platform shutdown. Jobs should be canceled by the plugin that scheduled them during shutdown: org.eclipse.tm.internal.terminal.emulator.VT100TerminalControl$1
Created attachment 187257 [details] Patch with fix. I saw the same problem with the process connector (getting a lot of error messages printed to the terminal when launching from command line). I think this patch should do the trick.
The patch looks reasonable, but what happens if you interrupt the Thread while the "Terminal data reader" job is inside its fTerminalText.processText() ? Is there a risk of disconnecting twice, getting into a loop, or logging the InterruptedException unnecessarily? It would seem cleaner to me to just do an fInputStream.close() -- the PipedInputStream that we use will set a closed status and notifyAll() so it should come out of the wait() while reading data. Effect should be the same of terminating the job more quickly, but also more cleanly. I didn't try this out, though.
Pawel, I have released a different change in the file that you patch. Could you update to HEAD, consider my comment #2, and upload a new patch?
Created attachment 190008 [details] Patch with fInputStream.close() I'm sorry I didn't reply earlier but I somehow missed your updates to the bug. (In reply to comment #2) > The patch looks reasonable, but what happens if you interrupt the Thread while > the "Terminal data reader" job is inside its fTerminalText.processText() ? > > Is there a risk of disconnecting twice, getting into a loop, or logging the > InterruptedException unnecessarily? Yes, no, yes. > It would seem cleaner to me to just do an fInputStream.close() -- the > PipedInputStream that we use will set a closed status and notifyAll() so it > should come out of the wait() while reading data. Effect should be the same of > terminating the job more quickly, but also more cleanly. I agree that using interrupt is rather heavy handed and closing the input stream is more appropriate. I guess my patch was more of a throw it over the fence approach ;-) Unfortunately calling PipedInputStream.close() does not cause the waitForAvaiable() to return immediately. So to make it work, I had to introduce a BufferedInputStream and call mark, read, reset, on it. I tested the patch and it appears to work, but it I'm not crazy about it.
Created attachment 190183 [details] Updated fix. This is an improved patch in that it doesn't add BufferedInputStream. Until I looked again now, I didnt' realize that the terminal implemented its own PipedInputStream. Given this I was able to modify the waitForAvailable() method to return immediately when stream is closed. (In reply to comment #4) > I tested the patch and it appears to work, but it I'm not crazy about it. I should qualify my statement a bit. The mark, read, reset is a valid paradigm and it's used here as the API intended. I'm not crazy about the previous patch because I had to introduce a BufferedInputStream into a solution where I don't fully understand the intent of the larger architecture. I still don't fully understand the intent of the architecture. For example I don't see why there is a custom PipedInputStream implementation and its javadoc doesn't help since it seems to be for a different module. I also couldn't find any unit tests which exercise the VT100TerminalControl, so I'm not fully confident that my changes won't break some use case that I'm not familiar with. So please review the patch with this context in mind.
CQ:WIND00260518
I found it unusual to have PipedInputStream#BoundedByteBuffer#available() return -1 if the Stream is closed, forcing the related small API change in line 246. It might be more correct to have PipedInputStream#BoundedByteBuffer#read() return -1 in line 144, and to modify PipedInputStream#waitForAvailable() to stop waiting if the Queue is closed in line 239. But I didn't have time to test or debug this, so I'm taking your word that this is OK. Released into 3.3m6 - I would appreciate your reviewing the committed change again based on above observations. Thanks for the patch.
BTW I ran the 209 unittests and 15 plugin-tests in o.e.tm.terminal.test and they all passed on my Win7 / J2SE-1.6.0_21 environment. I'm afraid though that none of those tests test the PipedInputStream
I'm actually reverting the release, since the PipedInputStreamPerformanceTest doesn't terminate any more (it looks like its "wt" writerThread doesn't like your change), and the PipedStreamTest doesn't continuously repeat. Both these tests are located in the o.e.tm.terminal.test project. Both are simple "Java Applications" that you can run in your workspace. Both work as expected again after reverting your change. Please revise.
Created attachment 191186 [details] Patch without available() api change. (In reply to comment #7) > I found it unusual to have PipedInputStream#BoundedByteBuffer#available() > return -1 if the Stream is closed, forcing the related small API change in > line 246. > > It might be more correct to have PipedInputStream#BoundedByteBuffer#read() > return -1 in line 144, and to modify PipedInputStream#waitForAvailable() to > stop waiting if the Queue is closed in line 239. Sure this works, and avoiding the API change also avoid breaking the performance tests. I tested this patch with a terminal connected to a process and with the performance test. IMO it would be more correct still to close the input stream immediately (instead of closing the queue), but I don't know whether the comment below on PipedInputStream.close() was deliberate or for convenience. /** * Closing a <tt>PipedInputStream</tt> has no effect. The methods in * this class can be called after the stream has been closed without * generating an <tt>IOException</tt>. * <p> */
Released the new patch for 3.3rc2 - thanks for the patch!
