Bugzilla – Full Text Bug Listing
| Summary: | When using Tomcat Default Servlet for directory listing - a negative file size is returned for big files | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [RT] Gemini.Web | Reporter: | Violeta Georgieva <milesg78> | ||||||||
| Component: | unknown | Assignee: | Violeta Georgieva <milesg78> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | glyn.normington | ||||||||
| Version: | 2.0.0.RELEASE | ||||||||||
| Target Milestone: | 2.1.0.M01 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 203770 [details]
Patch proposal
I reviewed the patch and the following code puzzles me slightly:
+ public long getContentLength() {
+ long size = this.bundleFileResolver .resolveBundleEntrySize(this.bundle, this.path);
+ if (size == 0) {
+ try {
+ size = getURL().openConnection().getContentLength();
+ } catch (IOException e) {
+ }
+ }
+ return size;
+ }
Why do the code not trust resolveBundleEntrySize to give the correct value?
The above "belt and braces" approach looks safe, but is actually difficult to test as you have to force the try block path to be executed for a valid bundle entry with non-zero size which returns 0 from resolveBundleEntry. Unless that code can be exercised by a testcase, it represents a kind of "time-bomb" in the code: if a user hits that path and for some reason the try block malfunctions, we end up with a hard to reproduce bug.
Also, if getContentLength possibly cannot be trusted, then it should be fixed, so better to rely on it and flush any bugs out.
On the other hand, if getContentLength is known to misbehave in some valid circumstances, the above code should have a comment explaining this and the need for the try block.
Comment on attachment 203770 [details]
Patch proposal
Setting review flag to - until my concerns are considered.
(In reply to comment #2) > Why do the code not trust resolveBundleEntrySize to give the correct value? If the BundleFileResolver is EquinoxBundleFileResolver then we can use equinox specific functionality to get BundleEntry and its size. When the BundleFileResolver is NoOpBundleFileResolver we will stick to the current implementation i.e. getting the bundle entry's size from the URLConnection. Wdyt? (In reply to comment #4) > (In reply to comment #2) > > Why do the code not trust resolveBundleEntrySize to give the correct value? > If the BundleFileResolver is EquinoxBundleFileResolver then we can use equinox > specific functionality to get BundleEntry and its size. When the > BundleFileResolver is NoOpBundleFileResolver we will stick to the current > implementation i.e. getting the bundle entry's size from the URLConnection. > > Wdyt? I see! Perhaps returning -1 as an indication that this is the NoOpBundleFileResolver would be less confusing since 0 is a valid file length. Created attachment 204011 [details]
Patch proposal
I agree with the comment.
I added also javadoc.
Thanks. Looks fine now. Change committed, tested and pushed Commit ID: 6a4557a6654dcd7374b58e2936761e8757b69119 |
Created attachment 203769 [details] Screenshot of the problem Hi, When file size (in kb) is bigger than max int then a negative size is returned (see attached screenshot). This is because we are using URLConnection to retrieve the bundle entry size. URLConnection.getContentLength() returns int as a result. So the corresponding BundleURLConnection.getContentLength() is the following: public int getContentLength() { return ((int) bundleEntry.getSize()); } I prepared a patch that is not based on URLConnection.getContentLength() when it is possible. Can you review it. Thanks and Regards Vily