Bug 370801

Comment 1 Helmut J. Haigermoser 2012-02-14 10:31:00 EST

Can we get a comment from the ECF side please, we are trying to create a p2 feature to pause an installation, and pausing the ECF download would have to work reliably to pull this off..

Thanks a lot! :)
Helmut

Comment 2 Scott Lewis 2012-02-14 10:36:48 EST

This bug represents an incomplete implementation of the pause functionality for the httpclient provider.  When pause is invoked, in addition to what it does now, it should also stop/end the existing transfer, and it currently does not.

Adding the complete pause should not be a big job technically, but there are not many resources currently for adding this functionality...so contributions are welcome.  I can/will guide the effort and help with testing in the short term.

Comment 3 Meng Xin Zhu 2012-02-15 01:22:11 EST

Hi Scott,

Could you give me a hint for starting to investigate a complete pause?
Thanks.
(In reply to comment #2)
> Adding the complete pause should not be a big job technically, but there are
> not many resources currently for adding this functionality...so contributions
> are welcome.  I can/will guide the effort and help with testing in the short
> term.

Comment 4 Scott Lewis 2012-02-16 12:56:41 EST

(In reply to comment #3)
> Hi Scott,
> 
> Could you give me a hint for starting to investigate a complete pause?
> Thanks.
> (In reply to comment #2)
> > Adding the complete pause should not be a big job technically, but there are
> > not many resources currently for adding this functionality...so contributions
> > are welcome.  I can/will guide the effort and help with testing in the short
> > term.

Sure.  First, upon second inspection, I may have been incorrect to assert that pause was not implemented...it does seem to be implemented.  So we're back to a bug in the httpclient provider.

So I need to ask:  In your tests, does the pause invoke *always* fail to pause...or does it sometimes stop things (even for large files)?  And:  Are you behind a proxy with these tests?

To understand what's going on, see/debug into the code in 

org.eclipse.ecf.provider.filetransfer.httpclient.HttpClientRetrieveFileTransfer.doPause()

This sets the 'pause' flag to true.

Then, in this job: 

org.eclipse.ecf.provider.filetransfer.retrieve.AbstractRetrieveFileTransfer.fileTransferRunnable.new IFileTransferRunnable() {...}.performFileTransfer(IProgressMonitor)

(the actual job that's doing the reading of the input stream) 

there is a test and loop:

while (!isDone() && !isPaused()) {
// read from input stream
// and deal with data
}

which...as you can see, should exit when pause is set.

then in the finally block is:

if (readInputStream != null) readInputStream.close();

and

hardClose();

And the http client file transfer impl of hardClose does this:

	protected void hardClose() {
		super.hardClose();
		if (getMethod != null) {
			getMethod.releaseConnection();
			getMethod = null;
		}
		responseCode = -1;
		if (proxyHelper != null) {
			proxyHelper.dispose();
			proxyHelper = null;
		}
	}

Which *should* stop the httpclient file transfer...but apparently it doesn't under some conditions.  I'm trying to reproduce what you are seeing now, and will continue to do so as I can over the next few days.  But if you are able to look at this as well, please put any info about what you find on this bug...I will do the same.

Comment 5 Scott Lewis 2012-02-16 14:41:36 EST

Hi Meng an others.

I think I've identified the problem.  

When the pause boolean is set, it results in this finally block getting executed (by the job/thread that runs org.eclipse.ecf.provider.filetransfer.retrieve.AbstractRetrieveFileTransfer.fileTransferRunnable.new IFileTransferRunnable() {...}.performFileTransfer(IProgressMonitor)


...
			} finally {
				try {
					if (readInputStream != null)
						readInputStream.close();
				} catch (final IOException e) {
					Activator a = Activator.getDefault();
					if (a != null)
						a.log(new Status(IStatus.ERROR, Activator.PLUGIN_ID, IStatus.ERROR, "hardClose", e)); //$NON-NLS-1$
				}
				hardClose();
				monitor.done();
...

The problem seems to be that for the readInputStream produced by httpclient, when close() is called, what happens is that the actual transfer *continues* and close blocks while this happens.  This is not what I would expect from calling InputStream.close() (rather I would expect that the underlying transfer would stop...which is what happens with other providers).

So, apparently we need to understand why readInputStream.close() blocks and continues the transfer...and make it so that it doesn't do this.  One way would be to simply provide an httpclient-specific wrapper for readInputStream (and implement just the close() method...e.g. to do nothing or something else that's the appropriate thing for httpclient).  I'll be investigating this over the next few days.  If anyone has further insights into this behavior InputStream.close() for httpclient 3.1 then please say so...perhaps it's some side effect of the use of the connection reuse logic (which I believe Meng was involved with, right?)

Comment 6 Scott Lewis 2012-02-16 16:36:09 EST

Ok...I've isolated the problem and have a fix.

It seems that the httpclient input stream responds to a call to InputStream.close() by continuing and completing the transfer, and blocking the InputStream.close() call until complete.

The expected method to halt an ongoing file transfer is getMethod.abort().  So restructuring the httpclient provider's hardClose() method to be this:

protected void hardClose() {
	// changed for addressing bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=370801
	if (getMethod != null) {
		// First, if !isDone and paused
		if (!isDone() && isPaused())
			getMethod.abort();
		// release in any case
		getMethod.releaseConnection();
		// and set to null
		getMethod = null;
	}
	// Close output stream...if we're supposed to
	try {
		if (localFileContents != null && closeOutputStream)
			localFileContents.close();
	} catch (final IOException e) {
		Activator.getDefault().log(new Status(IStatus.ERROR, Activator.PLUGIN_ID, IStatus.ERROR, "hardClose", e)); //$NON-NLS-1$
	}
	// clear input and output streams
	remoteFileContents = null;
	localFileContents = null;
	// reset response code
	responseCode = -1;
	// If we're done and proxy helper still exists, then dispose
	if (proxyHelper != null && isDone()) {
		proxyHelper.dispose();
		proxyHelper = null;
	}
}


With this new hardClose implementation, my tests show that the URLRetrievePauseResumeTest works fine, for both large and small files.  I've also made a small change to the test code, to download a 1M file, and to pause 10 seconds rather than just 4 seconds).  The fix was pushed to master.

http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=7b1c0c70b2c346588b442b1c8cdfe93f1e6fed81

Resolving this bug as fixed.  I will try to get this fix into p2/platform for Juno.  Thanks for the report.

Comment 7 Meng Xin Zhu 2012-02-17 03:04:02 EST

Hi Scott,
Thanks for your fix. I'll test it more from p2's perspective.
(In reply to comment #6)
> The fix was pushed to master.
> 
> http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=7b1c0c70b2c346588b442b1c8cdfe93f1e6fed81
> 
> Resolving this bug as fixed.  I will try to get this fix into p2/platform for
> Juno.  Thanks for the report.

Comment 8 Scott Lewis 2012-03-01 14:10:34 EST

Meng and others:

ECF is going to schedule a maintenance release (3.5.5) for March 19, 2012.  After this release (and probably after EclipseCon is over) we will request that the p2 team adopt this new version of ECF filetransfer for the Juno release.  This version 3.5.5...will have the pause fix discussed on this bug.  If you have any pull on the p2 team, please request that they adopt ECF 3.5.5...once it's complete.

And if there are any issues that you have found in testing pause with p2, we/I will have to know about them shortly in order to address them before 3.5.5.