Community
Participate
Working Groups
Build ID: M20090211-1700 (Problem still in HEAD) STEPS TO REPRODUCE Call DialogSettings.save() on a Writer that produced a IOException or with a file which causes trouble during output (e.g. full drive). EXPECTED The original exception thrown by the underlying output should be propagated to the caller -- as "save() throws IOException" suggests. ACTUAL The IOException is silently ignored. ANALYSIS The internal DialogSettings.XMLWriter is based in a java.io.PrintWriter, which catches all IOExceptions.
Created attachment 142267 [details] DialogSettingsTest.java Added a test case to reproduce the problem.
Created attachment 149360 [details] patch.txt This patch fixes the issue. The XMLWriter is now based on a BufferedWriter instead of a PrintWriter.
First of all, thanks for the test case and patch. Much appreciated! It makes fixing and verifying the bug much simpler. Except in this case there's an API breakage technicality. The problem I have is that this patch introduces a throws IOException clause into the public API for DialogSettings. public void save(Writer writer) becomes public void save(Writer writer) throws IOException I realize that the interface definition for IDialogSettings defines the throws clause, but today it's possible to write code like this: DialogSettings settings = new DialogSettings("test"); settings.save(myWriter); The patch means this code would no longer compile and clients would have to add the try/catch. If DialogSettings were an internal implementation class, this kind of change would be ok, but the implementation class is public, too. I noticed this because I wanted to run the old dialog settings against the new test case to verify the breakage, but I got an "unreachable catch block" error in the code, thus realizing I could compile code without the catch block in the old case, but not in the new case. To further complicate things, I checked the CVS history to see how long the implementation had existed without the throws clause. Turns out (sigh) that DialogSettings.save(Writer) has thrown an IOException since Eclipse was released, but this throws clause was removed early in 3.5 development to fix a compiler warning. So technically speaking, we relaxed the API in 3.5 (possibly causing downstream clients new compile warnings for unreachable catch blocks), and this patch would restore it to the 3.4 (and prior) method signature, forcing callers to catch the exception. Since we seem to have changed it casually in 3.5 (there is no bug number on the commit), perhaps we can casually change it back for 3.6. but I'd like to understand better the actual use case (what kind of writer were you using, etc.) to justify the change. The SDK uses the save(String fileName) method which is propagating the expected exceptions. cc'ing Boris for comment. Am I being too paranoid?
The Use Case comes from a major RCP deployment at the Swiss Railroad (SBB), where it tooks me some time to debug the reason, why dialog settings are sometimes not persisted. It's more or less a typical enterprise setup. 1) The workspace is on the user's home share (no write access elsewhere). 2) The users home share is a network share with quota. 3) The quota limit is reached. In this situation it is possible opening a FileOutputStream, but not writing to it. On the share a file with 0 bytes is created. AFAIK the network shares in our scenario are backed with Netapp appliances.
Moving to M4 as this is my last day to work on new stuff for M3. I consulted http://wiki.eclipse.org/Evolving_Java-based_APIs_2 for the rules of engagement here. It indicates that adding or removing checked exceptions from API methods is considered "Breaking" but the footnote says (1) Adding and deleting checked exceptions declared as thrown by an API method does not break binary compatibility; however, it breaks contract compatibility (and source code compatibility). But the bottom line is that we *already did this* in 3.5 by removing the throws clause and the correct behavior is to throw the exceptions and restore the clause. Boris, can you confirm if it's OK to break source compatibility from 3.5 to 3.6 if the breakage from 3.4 to 3.5 was unintentional/accidental?
Right, the change would be binary compatible but not source compatible. Specifically, clients moving from 3.4 to 3.5 will have to get rid of a try..catch if they recompile. And then, if they move from 3.5 to 3.6, they will have to add the try..catch again (if they recompile). Clients moving from 3.4 to 3.6 won't have to do anything. What a mess! Is there a way for clients to use IDialogSettings (and not DialogSettings) if they want to avoid the fallout? If yes, I would be leaning towards allowing the source incompatibility as you suggest, Susan, with an entry in the porting guide that gives recommendations for what to do.
(In reply to comment #6) > Is there a way for clients to use IDialogSettings (and not DialogSettings) if > they want to avoid the fallout? If yes, I would be leaning towards allowing the > source incompatibility as you suggest, Susan, with an entry in the porting > guide that gives recommendations for what to do. Fortunately the mainstream use cases (AbstractUIPlugin, Wizard, Dialog, etc.) that use dialog settings use IDialogSettings (I imagine that's why the source incompatibility snuck in during 3.5 without anyone noticing.) AbstractUIPlugin manages the creation, loading, and saving. The issue is simply that the concrete implementation is available as API, so it's possible for clients to run into this. I'll fix during M4.
Fixed in HEAD >20091202. I also added a section to the "incompatibilities" document explaining the source incompatibility. Thanks again, Marc.
Comment on attachment 149360 [details] patch.txt This patch contains the fix and test.
verified on WinXP, Build id: I20091208-0100 - checked source - checked incompatibilities guide - automated test case has been passing (and passed in I20091207-1800, results not available on this build yet)
