I took a stab at including allowing Util.readStreamAsString() to throw IOException, but I got to a point where I was touching lots of code and I chickened out on this patch.
The real point of this patch was to fix the bug in not closing the InputStream, which I need for incremental compile. Maybe another day. http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java (right): http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java#newcode139 dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java:139: InputStream in = resource.openContents(); On 2011/06/26 01:46:30, jbrosenberg wrote:
This Util.copy isn't needed to be inside the try catch.
Util.copy() throws IOException already, I was just moving the open of the stream inside. http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java File dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java (right): http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java#newcode294 dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java:294: InputStream stream = classSource.openContents(); On 2011/06/26 01:46:30, jbrosenberg wrote:
Util.readStreamAsString doesn't need to be inside try/catch
It calls Util.copy() then suppresses the IOException. I wanted to move it too, but thought it would make the patch too big. I added a TODO() http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/resource/Resource.java File dev/core/src/com/google/gwt/dev/resource/Resource.java (right): http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/resource/Resource.java#newcode84 dev/core/src/com/google/gwt/dev/resource/Resource.java:84: /** On 2011/06/26 01:46:30, jbrosenberg wrote:
This comment is no longer valid, no? It can't return null anymore.
But it
gives me pause, since it mentions the resource might be invalildated intentionally by its containing resource. Do we know what that's
about? Since
much of this change is to treat an IOException as a RuntimeException,
etc. (not
that the current behavior is essentially resulting in that anyway)...
It gave me pause too - it sounds like it might have something to do with HostedMode refresh. I did go through and trace back all uses of this method and could only find the one test where an invalid resource was expected and handled. - Util.copy() ends up choking on an NPE and throwing it - Util.readStreamAsString() calls Util.Copy() - in CompilationUnit builder, openContents() is fed unchecked to Util.copy() - in JavaSourceParser, calls Util.readStreamAsString() - in SourceFileCompiationUnit... well, I already know the NPE obscured a running out of filehandles issue. - in ResourceFactory, the problem trickles down several functions and then throws an NPE inside of a place that was catching an IOException and throwing a RuntimeException from it. - In StandardPublicResource an api function is created, but all references of StandardPublicResource.getContents() either assume blindly the method returned non null, or are already surrounded in IOException try/catch clauses. http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java File dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java (right): http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java#newcode75 dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java:75: On 2011/06/26 01:46:30, jbrosenberg wrote:
Not sure what this comment is supposed to be saying (and I relealize
it's not
your mod). 10 Gold stars if you can disambiguate!
Let me know what you think. http://gwt-code-reviews.appspot.com/1461803/diff/6002/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java File user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java (right): http://gwt-code-reviews.appspot.com/1461803/diff/6002/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java#newcode204 user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java:204: } catch (IOException iex) { On 2011/06/26 01:46:30, jbrosenberg wrote:
This changes the behavior, from being an NPE to essentially a UnableToCompleteException. I assume that's ok.
I thought it would be better to include the IOException in the log - that's what I was getting after in this patch. http://gwt-code-reviews.appspot.com/1461803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
