Bug 838681 - DriftFileSenderTest.doNotSendEmptyZipFile fails on java7
DriftFileSenderTest.doNotSendEmptyZipFile fails on java7
Status: CLOSED CURRENTRELEASE
Product: RHQ Project
Classification: Other
Component: Plugin Container (Show other bugs)
4.4
Unspecified Unspecified
high Severity unspecified (vote)
: ---
: RHQ 4.5.0
Assigned To: John Sanda
Mike Foley
:
Depends On:
Blocks: 682878 846049
  Show dependency treegraph
 
Reported: 2012-07-09 15:08 EDT by Heiko W. Rupp
Modified: 2013-09-01 06:18 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 846049 (view as bug list)
Environment:
Last Closed: 2013-09-01 06:18:27 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Heiko W. Rupp 2012-07-09 15:08:32 EDT
[17:47:26] <pilhuhn> jshaughn hey, I am looking at org.rhq.core.pc.drift.DriftFilesSenderTest#doNotSendEmptyZipFileToServer because this is failing for me
[17:48:14] <pilhuhn> now when I step into the code, I see the DritftFilesSender.run() using the DriftClientTestStub
[17:48:24] <pilhuhn> which in org.rhq.core.pc.drift.DriftClientTestStub#sendChangeSetContentToServer
[17:48:37] <pilhuhn> unconditionally increases the counter, which then gets checked in the test at
[17:48:51] <pilhuhn> assertEquals(driftClient.getSendChangeSetContentInvocationCount(), 0
[17:49:03] <pilhuhn> no it is pretty old, but I am looking into jdk7 and mvn 3
[17:54:10] <jshaughn> pilhuhn: so this is passing on Java6?


[19:53:10] <jshaughn> Heiko, ok, so there is a good reason why this test passes on Java6.  It's because Java6 throws an exception when trying to close a ZipOutputStream that has had nothing added to it.  This happens the line *before* driftClient.sendChangeSetContentToServer(), it's caught and handled with logging.
[19:56:34] <jshaughn> Given the lack of comments it almost seems like it was not expected by John but maybe it was.  Now, it seems that Java7 maybe allows the empty zip.  Looking for some doco on this
[20:10:28] <jshaughn> well I can't find anything saying this was explicitly changed although I wonder if maybe it's fallout from the new Zip FileSystem support
[20:12:09] <jshaughn> Anyway, I think this may require an actual code change if in fact J7 has started allowing empty zips

[21:02:51] <jshaughn> right, I haven't run locally with J7 but it seems that perhaps they now allow an empty zip
[21:03:09] <pilhuhn> Let me open a BZ for this
[21:03:20] <jshaughn> If you can call ZipOutputStream.close() and not get an exception then we have a problem
[21:04:21] <jshaughn> because that will in fact send the empty zip to the server, which, apparently, we don't want to do.  Although, I'm not sure why we prevent it.
[21:05:15] <jshaughn> I guess it's just an optimization because it would be a waste of time, I guess.  And perhaps there is server code that would not behave well
[21:05:51] <jshaughn> So, we will need to, I guess, not depend on the exception
Comment 1 Heiko W. Rupp 2012-07-17 18:14:12 EDT
There is clearly a difference in java 6 and 7

This fails on 6 and works on 7:


    @Test
    public void testEmptyZip() throws Exception {
        File file = new File("/tmp/my.zip");
        if (file.exists())
            file.delete();

        FileOutputStream fos = new FileOutputStream(file);
        ZipOutputStream zos = new ZipOutputStream(fos);
        zos.finish();
        zos.close();

    }

Did not yet find a bug reference though.
Comment 2 Charles Crouch 2012-08-02 15:33:54 EDT
Assigning to jsanda since it seems he was the original author of the drift related code.
Comment 3 John Sanda 2012-08-06 12:44:19 EDT
Since an exception is not thrown when closing a ZipOutputStream on an empty zip file in Java 7, I have added logic to check whether or not the zip file has any content. sendChangeSetContentToServer is called only if the content file is non-empty. The logging logic has been updated as well so that we only log an error message when failing to close the output stream only when the content file is non-empty. This keeps the logging clean and consistent across Java 6 and 7. Changes have been pushed to master.

commit hash: 5b37f7a17ef
Comment 4 Heiko W. Rupp 2013-09-01 06:18:27 EDT
Bulk closing of items that are on_qa and in old RHQ releases, which are out for a long time and where the issue has not been re-opened since.

Note You need to log in before you can comment on or make changes to this bug.