Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 369834 - Failure reading zipped repository is causing PDE/Build test failure
Summary: Failure reading zipped repository is causing PDE/Build test failure
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.8.0 Juno   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: Juno M6   Edit
Assignee: Meng Xin Zhu CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 368246
  Show dependency tree
 
Reported: 2012-01-26 11:49 EST by Andrew Niefer CLA
Modified: 2013-11-10 22:31 EST (History)
7 users (show)

See Also:


Attachments
File being read (2.30 KB, application/zip)
2012-02-08 07:53 EST, John Arthorne CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Niefer CLA 2012-01-26 11:49:38 EST
PDE/Build's test PublishingTests#testShape_267506 is failing in p2 while reading a zipped repository.

The file exists and appears the be valid.  I don't know the cause of this, stepping through the debugger caused the test to start passing locally.  Could be a timing issue.

Unable to read repository at
jar:file:/Users/ibmemployee/buildtest/I20120125-1800/eclipse-testing/test-eclipse/eclipse/pdebuild_folder/pde.build/publishShape/I.TestBuild/f-TestBuild-group.group.group.zip!/content.xml.

org.eclipse.equinox.p2.core.ProvisionException: Unable to read repository at
jar:file:/Users/ibmemployee/buildtest/I20120125-1800/eclipse-testing/test-eclipse/eclipse/pdebuild_folder/pde.build/publishShape/I.TestBuild/f-TestBuild-group.group.group.zip!/content.xml.
at
org.eclipse.equinox.internal.p2.repository.CacheManager.updateCache(CacheManager.java:359)
at
org.eclipse.equinox.internal.p2.repository.CacheManager.createCache(CacheManager.java:205)
at
org.eclipse.equinox.internal.p2.metadata.repository.SimpleMetadataRepositoryFactory.getLocalFile(SimpleMetadataRepositoryFactory.java:66)
at
org.eclipse.equinox.internal.p2.metadata.repository.SimpleMetadataRepositoryFactory.load(SimpleMetadataRepositoryFactory.java:88)
at
org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.factoryLoad(MetadataRepositoryManager.java:57)
at
org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.loadRepository(AbstractRepositoryManager.java:749)
at
org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.loadRepository(AbstractRepositoryManager.java:651)
at
org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:96)
at
org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:92)
at
org.eclipse.pde.build.internal.tests.p2.P2TestCase.loadMetadataRepository(P2TestCase.java:99)
at
org.eclipse.pde.build.internal.tests.p2.PublishingTests.testShape_267506(PublishingTests.java:1007)
at org.eclipse.pde.build.tests.PDETestCase.runTest(PDETestCase.java:61)
Comment 1 Ian Bull CLA 2012-01-26 16:46:18 EST
This particular stack trace has not happened in a recent IBuild. Also, both DJ and I have run the p2 tests locally and not seen this either.

However, there are a few tests that are failing related to unpacking. These happened recently on the 1.7 VM test machine.  Could be related to that.  I'll keep an eye out.
Comment 2 John Arthorne CLA 2012-02-07 22:17:29 EST
I can reproduce this. I traced it as far as:

org.eclipse.ecf.internal.provider.filetransfer.Activator#getFileTransfer(String protocol)

When asked for the protocol handler for the "jar" protocol, it comes up empty. We received a new ECF in the past month or so, is it possible they removed support for the "jar" protocol?
Comment 3 Scott Lewis CLA 2012-02-08 02:32:27 EST
Hmmm....No, we/ECF haven't done anything to remove support for the jar protocol.  The only thing in the most recent version was the addition of support for reusing connections in the httpclient provider.

Could someone post the jar that seems to be at issue...and I'll give it a try in ECF test code to see if I can reproduce.  Thanks.
Comment 4 John Arthorne CLA 2012-02-08 07:53:57 EST
Created attachment 210725 [details]
File being read

Here is the URI passed to the ECF file transfer API in my case:

jar:file:/C:/1target/junit-workspace/pde.build/publishShape/I.TestBuild/f-TestBuild-group.group.group.zip!/
Comment 5 John Arthorne CLA 2012-02-08 08:04:47 EST
I might have missed it, but I can't find an ECF extension for the retrieveFileTransferProtocolFactory extension point for the "jar" protocol in any of the ECF bundles we ship.
Comment 6 Scott Lewis CLA 2012-02-08 10:16:44 EST
(In reply to comment #5)
> I might have missed it, but I can't find an ECF extension for the
> retrieveFileTransferProtocolFactory extension point for the "jar" protocol in
> any of the ECF bundles we ship.

I believe it's handled by the jre URL protocol handler...as with file.
Comment 7 John Arthorne CLA 2012-02-08 17:33:09 EST
I created a simpler p2 test case with identical jar and it worked. So the problem is not so simple and doesn't seem related to ECF. Thanks for your quick comment Scott.
Comment 8 John Arthorne CLA 2012-02-08 17:48:28 EST
This bug was introduced by the fix for bug 364929. In that bug Meng introduced a static field on ProgressStatistics for the provisioning agent. The problem is, the agent cannot be static because there can be multiple agents running at once. In particular, p2 directory application creates its own agent, which is used in PDE build scenarios. Here is what is happening:

- PDE build runs, invoking p2 directory
- ProgressStatistics.agent gets set to the director's agent
- p2 director completes, and stops its agent
- We then try to perform a download, and it fails because the agent has been stopped. The IllegalStateException gets swallowed and it just appears to be a transfer failure.

For reference here is the stack trace of the underlying failure:

ProvisioningAgent.checkRunning() line: 85	
ProvisioningAgent.getService(String) line: 49	
ProgressStatistics.publishEvent(DownloadProgressEvent) line: 58	
ProgressStatistics.report() line: 147	
FileReader.handleTransferEvent(IFileTransferEvent) line: 164	
UrlConnectionRetrieveFileTransfer(AbstractRetrieveFileTransfer).fireReceiveStartEvent() line: 671	
UrlConnectionRetrieveFileTransfer.openStreams() line: 330	
UrlConnectionRetrieveFileTransfer(AbstractRetrieveFileTransfer).sendRetrieveRequest(IFileID, IFileRangeSpecification, IFileTransferListener, Map) line: 889	
UrlConnectionRetrieveFileTransfer(AbstractRetrieveFileTransfer).sendRetrieveRequest(IFileID, IFileTransferListener, Map) line: 576	
MultiProtocolRetrieveAdapter.sendRetrieveRequest(IFileID, IFileTransferListener, Map) line: 106	
FileReader.sendRetrieveRequest(URI, OutputStream, FileReader$DownloadRange, boolean, IProgressMonitor) line: 349	
FileReader.readInto(URI, OutputStream, long, IProgressMonitor) line: 295	
RepositoryTransport.download(URI, OutputStream, long, IProgressMonitor) line: 87	
RepositoryTransport.download(URI, OutputStream, IProgressMonitor) line: 137	
CacheManager.updateCache(File, URI, long, SubMonitor) line: 333	
CacheManager.createCache(URI, String, IProgressMonitor) line: 205	
SimpleMetadataRepositoryFactory.getLocalFile(URI, IProgressMonitor) line: 66	
SimpleMetadataRepositoryFactory.load(URI, int, IProgressMonitor) line: 88	
MetadataRepositoryManager.factoryLoad(URI, IExtension, int, SubMonitor) line: 57	
MetadataRepositoryManager(AbstractRepositoryManager).loadRepository(URI, String, String, int, SubMonitor) line: 749	
MetadataRepositoryManager(AbstractRepositoryManager).loadRepository(URI, IProgressMonitor, String, int) line: 651	
MetadataRepositoryManager.loadRepository(URI, int, IProgressMonitor) line: 96	
MetadataRepositoryManager.loadRepository(URI, IProgressMonitor) line: 92	
PublishingTests(P2TestCase).loadMetadataRepository(URI) line: 99
Comment 9 John Arthorne CLA 2012-02-08 17:56:10 EST
Meng, can you take a look at fixing your work on ProgressStatistics? This class cannot have a static agent. I see you are setting it in some places before calling a repository, but there could be multiple threads in play. A couple of options:

- Agent gets passed in to the Transport and set as an instance field on ProgressStatistics
- Only use the agent of the currently running system - this means you won't get progress statistics for multi-agent scenarios such as the p2 director application.
Comment 10 John Arthorne CLA 2012-02-08 18:03:18 EST
I have released a p2 test case that illustrates the problem:

http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=5d2d6401627c5851207411a25c44dd56c17958da

See JarURLMetadataRepositoryTest._testZipFileRepository
Comment 11 Pascal Rapicault CLA 2012-02-08 20:25:52 EST
At this point I propose that we revert the commit from bug #364929. Aside from the problem it causes now, I disagree with the approach taken to pass in this parameter and litters the API with very specific fields.
We should discuss a better / more generic approach (my bad for not doing it earlier).
Comment 12 Meng Xin Zhu CLA 2012-02-08 22:38:47 EST
John, I'll take it.

The change of ProgressStatistics is NOT for bug 364929, it's made for bug 316328. The download progress was reported by download thread to update IProgressMonitor with predefined format. It's hard to be read by users if there are multiple downloading threads. The text is quickly updated by each thread, no overall download speed and estimated time.

The change of ProgressStatistics is intend to broadcast the downloading progress via ProvisioningEventBus. It's originally designed as utility class, I'll try to make it suit multiple agents.
(In reply to comment #8)
> This bug was introduced by the fix for bug 364929. In that bug Meng introduced a
> static field on ProgressStatistics for the provisioning agent. The problem is,
> the agent cannot be static because there can be multiple agents running at once.
> In particular, p2 directory application creates its own agent, which is used in
> PDE build scenarios. Here is what is happening:
> 
> - PDE build runs, invoking p2 directory
> - ProgressStatistics.agent gets set to the director's agent
> - p2 director completes, and stops its agent
> - We then try to perform a download, and it fails because the agent has been
> stopped. The IllegalStateException gets swallowed and it just appears to be a
> transfer failure.
>
Comment 13 Meng Xin Zhu CLA 2012-02-09 06:15:29 EST
I submitted a fix[1]. Pls help review it. Thanks.

Each agent has its Transport service, so the implementation of Transport could have the instance of agent which creates it. And somewhere are using RepositoryTransport as an utility class to download content, hence the agent is not mandatory for constructing an instance of RepsotiroyTransport. Regarding to P2 provisioning operations they get Transport service from agent, it means that the agent always is available for Transport service used by provisioning. Download progress events can be broadcasted during the provisioning.

[1] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=91027ecca979ba174a9617241ff3999250490ae9
Comment 14 John Arthorne CLA 2012-02-09 14:47:47 EST
(In reply to comment #13)
> I submitted a fix[1]. Pls help review it. Thanks.
> 
> Each agent has its Transport service, so the implementation of Transport could
> have the instance of agent which creates it. And somewhere are using
> RepositoryTransport as an utility class to download content, hence the agent is
> not mandatory for constructing an instance of RepsotiroyTransport. Regarding to
> P2 provisioning operations they get Transport service from agent, it means that
> the agent always is available for Transport service used by provisioning.
> Download progress events can be broadcasted during the provisioning.
> 
> [1]
> http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=91027ecca979ba174a9617241ff3999250490ae9

This fix looks good to me. It removes all statics and ensures the progress statistics class is publishing to the appropriate agent for that instance.
Comment 15 Ian Bull CLA 2012-02-09 23:27:37 EST
Thanks John for all the help on this bug, and Meng, thanks for getting a patch together so quickly.  The patch looks good in that it removes the uses of ServiceHelper to get static agent instances.  

I'll merge this to our integration branch.

Pascal, if you want to review / discuss  bug #364929, let's take that onto another bug report.
Comment 16 Meng Xin Zhu CLA 2012-02-15 21:40:54 EST
Close it.