On Sun, Feb 10, 2008 at 04:23:07AM -0800, Mark Brouwer (JIRA) wrote: > > [ > https://issues.apache.org/jira/browse/RIVER-9?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel > ] > > Mark Brouwer resolved RIVER-9. > ------------------------------ > > Resolution: Fixed > > Applied code modification, given no objection I assume people have reviewed > but forgot to mention they liked what they saw.
Sorry for the belated review. Changes look fine, with one minor quibble about this comment change: - * causing a duplicate request of the JAR file on first - * use. (For HTTP-protocol URLs, the initial request will - * use HEAD instead of GET.) + * causing duplicate requests of the JAR file on first use when + * our first attempt fails. (For HTTP-protocol URLs, the initial + * request will use HEAD instead of GET.) With these changes, it will not be the "initial" request that uses HEAD instead of GET-- rather, it would just be the fallback request to determine existence of the JAR file if the initial request fails. Like I said here, however: http://mail-archives.apache.org/mod_mbox/incubator-river-dev/200701.mbox/[EMAIL PROTECTED] I still can't completely explain why the separate initial check for the existence of the JAR file is even necessary. There was some discussion elsewhere in that river-dev thread that the problem was about not being able to distinguish FileNotFoundException occurring because the JAR file definitely does not exist (like after HTTP response 404) from FNFE occurring because the JAR file exists but contains no META-INF/PREFERRED.LIST entry-- but I don't see how that could be the issue here, because while an FNFE may be an ambiguous failure, PreferredClassLoader.isPreferredResource just wants to return false in either case anyway. I thought that it had something to do with the "jar:" URL handler, in some circumstances at least, not necessarily throwing FileNotFoundException, but instead some other IOException, in the case of an HTTP 404 response-- hence the HTTP response code analysis in PCL.getPreferredConnection. But I can't say sure what relevant circumstances would cause that. A basic test I just ran against a range of JDK versions shows that attempting to retrieve contents of a non-existent JAR file through a "jar:" URL does throw a non-FNFE IOException in 1.2.x, but it does seem to throw a FNFE in 1.3 and later, and I would not have thought that PreferredClassLoader's initial implementation was intended to accommodate running on JDK 1.2.x. But there are very likely cases that I'm not thinking of... The other option that I can think of is that PreferredClassLoader was trying to be careful to only consider 4xx HTTP responses to indicate that the JAR file definitely does not exist, not 5xx HTTP responses, which are logically less definitive, but still cause FileNotFoundException. This seems like a plausibile explanation, but it doesn't ring a bell with me (Bob?). At any rate, even though I can't completely explain the need for the separate existence check, I wouldn't advocate removing it altogether without much more consideration. In the meantime, your changes seem reasonable to me. -- Peter
