Hi All, I don't have much time now, but what I can say is that clearing the event reference in the repository was very important when the event was keeping a reference to the byte array transfered. Since it's no longer the case, keeping the reference to the transfer event is not too much an issue, and is thus a pragmatic solution.
Now for the FileUtil and CopyProgressListener part, the documentation doesn't say anything, but I think that the intent is to send an event only if the transfer is successfully completed. Maybe adding an event in case of an error would improve the API, and then to avoid modifying the CopyProgressListener interface we could simply call end and add an exception in the CopyProgressEvent. But if we want to call the end method only if the copy is actually successful, putting the call after closing the streams would cause it to be sent if an exception occured in the try block. So we should remind to test if we have actually reached the end of the try block before calling the end method. Or test if no exception was raised, which is better if we want to call the end method in all cases, with an exception in case of a transfer which caused an exception, and without in case of a successful transfer. What do you think? Xavier On 1/5/07, Maarten Coene <[EMAIL PROTECTED]> wrote:
Hi Stephane, you're absolutely right. The main problem is the semantics. I considered the end as the "end of transfer", not as the "end of publish" or "end of retrieve" phase. I consider "transfer a file" as only copying the file to another location, where "publish a file" or "retrieve a file" is much more. Of course, I could be wrong here. For instance, some checks are performed after the transfer to make sure the artifact has been correctly copied. In this case, the filesize was compared (I think the IOException was thrown there, not in the close() call), but it could also be some checksum verification that failed. In my opinion, this had nothing to do with the file transfer, which is completed at this stage. But I think you are also right about the FileUtil.copy() method, which should probably call end() after closing the streams, although this wouldn't avoid the NPE in all situations. But as you say, it's not very clear from the code nor the documentation what is exactly meant by the "transferCompleted" event. Maybe Xavier can give us some more insight? regards, Maarten ----- Original Message ---- From: Stephane Bailliez <[EMAIL PROTECTED]> To: [email protected] Sent: Friday, January 5, 2007 12:34:24 AM Subject: Re: svn commit: r492785 - in /incubator/ivy/trunk: CHANGES.txtsrc/java/fr/jayasoft/ivy/repository/AbstractRepository.java Hi there, I'm not sure you've been fixing the right thing (even though you'll avoid the npe). It looks like to me the root of the problem is in FileUtil.copy(). I suspect the IOException happened in the finally with the close() calls and as the end() call is performed before, this nulled the reference (as far as I can read the reference is nulled only after a end or error.) So if you consider that an IOException in the close should make the transfer fail, then the end() of the transfer would be after the close(). Whether or not you put the end() call in the finally or not depends whether end you consider that end is actually success or not and then where it makes sense to cleanup the event reference (if any). It does not seem extremely clear in the code and the semantic is not documented in the interface, but considering the reference was nulled in both end and error, I suspect end meant success. in that case I'll put the end() call after the finally. But in any case it cannot be end followed by error if end is meant as success. I may be of course completely wrong as my head spins easily after a double callback and nested calls. :) -- stephane [EMAIL PROTECTED] wrote: > Author: maartenc > Date: Thu Jan 4 14:38:49 2007 > New Revision: 492785 > > URL: http://svn.apache.org/viewvc?view=rev&rev=492785 > Log: > FIX: IOException during publish causes NullPointerException (IVY-371) > > Modified: > incubator/ivy/trunk/CHANGES.txt > incubator/ivy/trunk/src/java/fr/jayasoft/ivy/repository/AbstractRepository.java > > Modified: incubator/ivy/trunk/CHANGES.txt > URL: http://svn.apache.org/viewvc/incubator/ivy/trunk/CHANGES.txt?view=diff&rev=492785&r1=492784&r2=492785 > ============================================================================== > --- incubator/ivy/trunk/CHANGES.txt (original) > +++ incubator/ivy/trunk/CHANGES.txt Thu Jan 4 14:38:49 2007 > @@ -10,6 +10,8 @@ > - IMPROVE: Please typedef CacheResolver as "cache" for us (IVY-359) > - IMPROVE: ivy:retrieve should be able to create symlinks (IVY-353) (thanks to John Williams) > > +- FIX: IOException during publish causes NullPointerException (IVY-371) > + > version 1.4.1 - 2006-11-09 > ===================================== > - IMPROVE: ability to rebuild all dependent projects from a leaf (IVY-101) > > Modified: incubator/ivy/trunk/src/java/fr/jayasoft/ivy/repository/AbstractRepository.java > URL: http://svn.apache.org/viewvc/incubator/ivy/trunk/src/java/fr/jayasoft/ivy/repository/AbstractRepository.java?view=diff&rev=492785&r1=492784&r2=492785 > ============================================================================== > --- incubator/ivy/trunk/src/java/fr/jayasoft/ivy/repository/AbstractRepository.java (original) > +++ incubator/ivy/trunk/src/java/fr/jayasoft/ivy/repository/AbstractRepository.java Thu Jan 4 14:38:49 2007 > @@ -65,7 +65,6 @@ > _evt.setTotalLengthSet(true); > } > fireTransferEvent(_evt); > - _evt = null; > } > > protected void fireTransferCompleted(long totalLength) { > @@ -73,20 +72,17 @@ > _evt.setTotalLength(totalLength); > _evt.setTotalLengthSet(true); > fireTransferEvent(_evt); > - _evt = null; > } > > protected void fireTransferError() { > _evt.setEventType(TransferEvent.TRANSFER_ERROR); > fireTransferEvent(_evt); > - _evt = null; > } > > protected void fireTransferError(Exception ex) { > _evt.setEventType(TransferEvent.TRANSFER_ERROR); > _evt.setException(ex); > fireTransferEvent(_evt); > - _evt = null; > } > > protected void fireTransferEvent(TransferEvent evt) { > > > > __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
