Bugzilla – Full Text Bug Listing
| Summary: | TerminalServiceShellOutputReader logs error when hostShell.exit() is called | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] Target Management | Reporter: | Rob Stryker <stryker> | ||||||||
| Component: | RSE | Assignee: | Martin Oberhuber <mober.at+eclipse> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | adietish, aleherb+eclipse, bfitzpat, manderse, nboldt | ||||||||
| Version: | 3.2 | ||||||||||
| Target Milestone: | 3.3 M5 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 335415 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Rob Stryker
Created attachment 187331 [details]
Patch to cancel the readers
Is there to be a 3.2.3 release? If so, our adopter product would appreciate very much if this could be in it.
For your patch to reliably work on a multiprocessor system, the minimum you need to do is declare the "TerminalServiceShellOutputReader.isCanceled" variable volatile (otherwise there's risk that the running Thread uses cached Thread-local storage and never reads the change to isCanceled).
The other thing I'm not too fond about is that
"TerminalServiceShellOutputReader.stopThread()"
doesn't really stop the Thread but just swallow exceptions, the method either isn't named right or doesn't quite the right thing, or needs to be documented to tell why it does what it does.
FYI, the problem seems similar to what we recently fixed for CDT in bug 286162 , perhaps reading that up gives you some ideas.
StopThread does stop the thread. In case you missed this part of the patch:
- while (!done && !isFinished()) {
+ while (!done && !isFinished() && !isCanceled) {
Calling stopThread() will cause the while loop to end, and thus the thread to end. If there happens to be an error during that last iteration, it will swallow the error and return null. If there is no error during that last iteration, the loop will end and the thread will end.
Would the patch be accepted if I added the 'volatile' to the canceled member variable? I can adjust the patch and re-attach if you would accept it.
You seem to assume that fReader.read() eventually terminates with an IOException because the connection to the target is closed. This is the case in your scenario (where the connection has already been closed). But if you name your method "stopThread()" it should also stop the thread in the case where the remote connection is not yet terminated ... in this case, the Thread could hang in read() forever since read() is a blocking call. So technically your stopThread() could not do what a user of the API would expect. See the TerminalServiceShelLWriterThread for example, its notifyAll() ensures that the Thread returns from its wait(). I'm not going to push the limits too high for accepting a patch, but I cannot guarantee accepting any patch before I see the patch. Adding a Unittest for the functionality would clearly help though I'm not going to make this a must-have. See o.e.rse.tests for existing tests that we have. Created attachment 187492 [details]
Makes suggested changes
I've attached a new patch. It *does* change the field in TerminalServiceShellWriterThread to volatile, as it changes the reader to volatile as well. I've also added the notifyAll();
I really don't have time to make a test case (sorry), and I don't care if you use my patch or give me iplog or whatever. Our adopter product just doesn't want random errors in the error log popping up when there's clearly no error ;)
Anyway, just let me know if the issue will be fixed, with or without my patch =]
Thanks.
feel free to change or modify the patch as you see fit to fix the issues to your satisfaction.... =] Created attachment 187503 [details]
patch v3
Here is a patch that I would accept:
- Fix Copyright comments
- Add Javadoc for stopThread()
- Use Thread.interrupt() to force return from blocking read() or Thread.sleep()
Can you please test whether this does what you want - I don't have the environment here for testing. Thanks.
Martin: Thanks so much for the patch. It works in my environment. No error logged. Thanks again! Do you think this can make it into Helios SR2? Released for 3.3m5. Thanks for the initial patch. Hey Martin: Do you think this can make it into Helios SR2, also? Thanks. Unlikely. We have RC2 today, and I have some other issues to backport first. Also, our rampdown policy forces me to ask another committer to review the patch for the backport. Being very strict on backports makes sense, because for bulk of the consumers SR2 is the last version they'll get. If we mess it up there's no easy fixing. I understand your point of view, but if you can manage to squeeze it in it would make some adopter products very happy. Many adopter products often times only use service releases, trusting them as more stable and cleaned up without many of the bugs that might be present in a x.y.0.GA. If you have the time, and can find it in your heart, or if you have a hotbug request policy, I'd appreciate it immensely if we could get this in. I can't promise. As mentioned, there are other issues too, and after all this is just a cosmetic problem. We could certainly use this for our own RSE functionality... +1 for less confusing log output and happier users. (In reply to comment #0) > Readers still log > an error to the error log view. This makes whatever action was called seem to > have failed with an error, and confuses users. (In reply to comment #11) > Unlikely. We have RC2 today, and I have some other issues to backport first. > Also, our rampdown policy forces me to ask another committer to review the > patch for the backport. Being very strict on backports makes sense, because for > bulk of the consumers SR2 is the last version they'll get. If we mess it up > there's no easy fixing. Seems that this is a fairly simple thing to fix - esp. since you've yourself described it as merely cosmetic - but given the impact on users to avoid unnecessary confusion and cruft in their logs, I'd argue that this is a perfect time to squeeze it in and save users from worry. Sure, it's just log clutter, but to most n00bs a clean log is a good log and anything else means they've broken something. Is it wrong to want to keep things clean for them? That said I understand there are time & quality constraints at play here. :) Ok, here's a deal: I release into Helios SR2 RC2, and you guys commit to toroughly testing the TM Terminal in that build? That's a win-win situation since you'll also test my other fixes :) Once there's a build I'll definitely test it =D http://download.eclipse.org/tm/downloads/drops/M20110125-2215/index.php has the backport. p2 repo / update site is at http://download.eclipse.org/tm/updates/3.2interim Ping, any results from testing? - Please set status VERIFIED when satisfied. Everything seems to work fine =D Specifically, My use case works fine, and I've not noticed any new issues crop up that I wasn't already aware of. So, thumbs up from me =] Thanks Rob! |