Bug 838681

Summary:	DriftFileSenderTest.doNotSendEmptyZipFile fails on java7
Product:	[Other] RHQ Project	Reporter:	Heiko W. Rupp <hrupp>
Component:	Plugin Container	Assignee:	John Sanda <jsanda>
Status:	CLOSED CURRENTRELEASE	QA Contact:	Mike Foley <mfoley>
Severity:	unspecified	Docs Contact:
Priority:	high
Version:	4.4	CC:	hrupp
Target Milestone:	---
Target Release:	RHQ 4.5.0
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:
Clones:	846049 (view as bug list)		Environment:
Last Closed:	2013-09-01 10:18:27 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	682878, 846049

Description Heiko W. Rupp 2012-07-09 19:08:32 UTC

[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 22:14:12 UTC

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 19:33:54 UTC

Assigning to jsanda since it seems he was the original author of the drift related code.

Comment 3 John Sanda 2012-08-06 16:44:19 UTC

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 10:18:27 UTC

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.