Hello Andy,
I really appreciate your effort in trying to explain this, but ...
What is missing for me in your approach is that it is not possible to
signal errors to the FtpClient in case something went wrong on the close, which
is crucial for me. I don't want to do some cleanup routine, I want to inform
the user that the transfer failed.
I also think the close should be done as soon as possible. Postponing it shows
that you assume the errors that occur are not significant (the logging and the
statistics will assume success).
A whole other discussion is whether to use close on the FileObject.
Calling the FileObject.close before OutputStream.close seems to me just a way
to give the FileObject a more direct way to handle the exceptions that might
occur when closing streams. If you do it your way, the close operation has to
switch between closing an inputstream or outpustream because in the scope of
the close itself the context is not clear.
As an alternative you could add a wrapper to the streams that do a specific
callback on the FileObject on close; the FileObject could have methods like
onCloseOutputStream(OutputStream) and onCloseInputStream() that are called by
those wrappers. Since the FileObject is responsible for creating the streams
this all works well.
E.g. something like:
public class OnCloseCallbackOutputStream extends FilterOutputStream {
private FileObject fileObject;
public OnCloseCallbackOutputStream(OutpuStream stream, FileObject fileObject)
{
super(stream);
this. fileObject = fileObject
}
public void close() throws IOException {
this.fileObject.onCloseOutputStream(this);
}
}
The close on the outputstream is still the most significant event in my view,
and whatever you decide to implement I hope you pass the exceptions.
Frank
> Date: Thu, 8 May 2008 06:24:44 -0600
> From: [EMAIL PROTECTED]
> To: [email protected]
> Subject: Re: (FTPSERVER-119) STOR command should not eat exceptions when
> closing stream
>
> Frank,
>
> Looks like I was not so clear on my presentation of the close(). I'll
> have another go at it. I think some of the problem is the concept of
> just what a close() call is. In this particular case it's just an
> providing a FileObject an opportunity to do some additional processing,
> it does not mean that the FTP server is finished with the FileObject.
>
> Its easier to visual sometimes, so here is the ftp server in a
> basic-sense. I only put in some of the major components, notice where
> the FileObject is, and notice where the commands like STOR are:
>
> +-------------------------------------------------------------+
> | FTP Server |
> +-------------------------------------------------------------+
> +-------------------------------------------------------------+
> | Server Context |
> +-------------------------------------------------------------+
> +----------+ +----------+ +------------+ +---------+ +--------+
> | Command | | Message | | FileSystem | | User | | Config |
> | Factory | | Resource | | Manager | | Manager | | |
> +----------+ +----------+ +------------+ +---------+ +--------+
> | |
> +----------+ +------------+
> | Commands | | FileSystem |
> +----------+ | View |
> | STOR | +------------+
> +----------+ |
> | ... | +------------+
> +----------+ | FileObject |
> +------------+
>
> In the over-all scheme of the ftp server, the commands can be seen as an
> intermediate level of a large operation. In most cases the commands are
> some-what detached from the rest of the server operations. This is
> similar to a large car factory with an assembly-line. The workers there
> do not know if any particular car is special and should be handled
> differently. The workers follow a pattern, repeating it for each car,
> perhaps it putting in a part, or a bolt. They are not aware of how the
> car got to them, nor what happens to the car after it leaves them. They
> just do their repetitive task, same as the ftp-server commands.
>
> In the FTP Server, the FileObject is a bit smarter than the "cars" and
> likely the workers too :-). It knows how to handle most tasks that
> involve it, such as creating input and output streams. It also knows
> whether it's a directory or a file, and if a directory, it can list's it
> children. It knows it's size or can obtain it, and so forth.
>
> In the current code-set, commands that use a FileObjects stream's will
> call a stream close() at the end of the command. The command STOR is an
> example of one of these commands. At the end of STOR, the close() is in
> a final block, to insure that the close() will always happen.
>
> public class STOR {
> ...
> } finally {
> IoUtils.close(outStream);
> }
> }
>
> The IoUtils.close() traps and ignores any exception that occurred with
> the close().
>
> In most cases the above scenario is okay, however in some cases,
> ignoring exceptions on a close() is not okay. How to handle these?
>
> One proposal is to add a FileObject close() method. Why? Because the
> command -level operations like STOR have no idea about what to do for
> any particular implementation of a FileObject. Who knows best about the
> FileObject? the FileObject itself. So by adding a close() to the
> FileObject, allows the FileObject the opportunity to do some additional
> processing. One such opportunity is to close it's streams and handle any
> exceptions.
>
> Understand that a FileObject does not have to use the close() method, it
> can just be empty. In any case the command-level operation, such as STOR
> will call it anyway. Just to be sure that the stream really is closed,
> the command-level operation will always call close() on the stream. If
> the FileObject closed the stream, it's okay, the stream close() ignores
> all exceptions.
>
> The close() proposal would add the addition close() in the final block
> of commands that use a FileObject stream. In addition, the order would
> be to call the FileObject close() first and then the Stream close. This
> will give the FileObject an opportunity to "personally" deal with any
> tasks that it needs, such as closing a stream and catching the exceptions.
>
> public class STOR {
> ...
> } finally {
> // let the FileObject do some post-process tasks
> IoUtils.close(file);
>
> // as-a-safety-check, try to close the stream
> IoUtils.close(outStream);
> }
> }
>
> Hopefully all of this makes sense? Again, keep in mind that the close()
> is always done at the end of a command. In the current code only a
> stream close is done, but it's always done at the end of the command. At
> that point the command [STOR as an example] is done with the FileObject.
>
> Also keep in mind that the close() in a FileObject does not have to do
> anything. More importantly, calling the close() does not mean the FTP
> Server is finished with the FileObject. The close() is just giving the
> FileObject an opportunity to do some house-cleaning if it wants to.
>
> Andy Thomson
>
>
> Frank van der Kleij wrote:
> > Hi Andy,
> >
> > I don't think it's conceptually clean to close the file object before
> > the stream is closed, it's very counterintuitive. I think the stream
> > must be closed first and than the file object. The close on the stream
> > means the transfer is finished, the close on the file object means you're
> > done with it altogether (in theory releasing it to an object pool or
> > something similar).
> >
> >
> > In the handling you propose the exceptions are ignored anyway
> > (IOUtils.close(FileObject)) so it does not resolve the need to inform the
> > FTP client that the file transfer was not successful.
> >
> > I do think that a close on FileObject is necessary but I don't think it is
> > a solution to my problem...
> >
> > I don't quite follow your explanation of the assembly-line since I'm new
> > here. In my view the outputstream is written and closed in the context of
> > the STOR operation using a particular FileObject. If the close on the
> > stream fails, the exception should be caught and an error code should be
> > returned to the client. Likewise, if the close on the FileObject fails an
> > error should be returned to the client too.
> >
> > If you don't want to be bothered by exceptions on close I think it should
> > be the responsibility of the FileObject to muffle them; e.g. by wrapping
> > the OutputStream and ignore the exceptions on close there. In the current
> > situation the STOR command is explicitly killing all options to signal
> > something went wrong.
> >
> > Frank
> >
> >
> >
> >
> >> Date: Wed, 7 May 2008 21:18:14 -0600
> >> From: [EMAIL PROTECTED]
> >> To: [email protected]
> >> Subject: Re: (FTPSERVER-119) STOR command should not eat exceptions when
> >> closing stream
> >>
> >> Frank,
> >>
> >> What should probably happen is that the FileObject.close() should be
> >> called first, this then gives you the opportunity to close the stream
> >> and handle the errors. The outStream close() would be called anyway,
> >> because not everyone may want to handle their own close() so it insures
> >> the stream is closed.
> >>
> >> In the STOR command
> >>
> >> ...
> >> finally {
> >>
> >> // provide an opportunity for the FileObject to handle close
> >> IoUtils.close(file);
> >>
> >> // close the stream, it may have been previously closed, make sure
> >> IoUtils.close(outStream);
> >> }
> >>
> >> In the FileObject
> >>
> >> public void close() {
> >> // close the stream(s) or raf
> >>
> >> // do some other clean-up
> >> }
> >>
> >> This implies some changes in the FileObject issuance of the
> >> createOutputStream() and createInputStream() if you wish to handle the
> >> close on the stream(s). Or at least a change for the write-stream in any
> >> case.
> >>
> >> Once an operation [close() for example] leaves the FileObject, ie, it's
> >> at the Command-level, the command-level does not know one file object
> >> from another, nor if one FileObject is suppose to be handled
> >> differently, it's just doing an assembly-line operation.
> >>
> >> Does the above make sense on why a FileObject.close() was proposed?
> >> Trying to handle individual FileObject tasks at the command level just
> >> causes the "assembly-line" to slow down, and possibly break. Far better
> >> to call the FileObject and let it deal with it's needs.
> >>
> >> From your comments [excellent feedback], what should be done at the
> >> command-level is call the FileObject.close() first, then the stream close.
> >>
> >> Andy Thomson
> >>
> >> Frank van der Kleij wrote:
> >>> Thanks for commenting on the issue.
> >>>
> >>> The close on the file object seems a good idea in general but for me it
> >>> won't do the trick. The Apache VFS API does provide for close operations
> >>> but they serve a slightly different purpose.
> >>>
> >>> VFS uses a stream based API to write; three objects are involved, a
> >>> FileObject, a FileContent and an OutputStream ( to write you'd have to do
> >>> fileObject.getFileContent().getOutputStream() ). Close on FileObject
> >>> means you're done treating the file, which is different from meaning that
> >>> you're done writing to it. The same goes for close on FileContent, which
> >>> is supposed to release all resources and closes InputStreams as well.
> >>> Only close on the OutputStream only means that all data is transferred.
> >>>
> >>> For information, internally in VFS operations, e.g. on a copy operation,
> >>> the close is only called on the outputstream too.
> >>>
> >>> My point is that a close on the FileObject is not what I'm looking for;
> >>> it is rather the handling of exceptions on close of the stream that
> >>> concern me.
> >>>
> >>> I doubt it is really necessary to ignore exceptions on the close of the
> >>> stream. I can imagine it was done because the close is called in a
> >>> finally block and handling exceptions there is rather ugly.
> >>>
> >>> By doing the close before the finally the exceptions can be handled
> >>> normally. It would do a double close on the same stream, but streams are
> >>> supposed to support that kind of thing.
> >>>
> >>> Best regards,
> >>> Frank
> >>>
> >>>
> >>> _________________________________________________________________
> >>> Express yourself instantly with MSN Messenger! Download today it's FREE!
> >>> http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
> >
> > _________________________________________________________________
> > Express yourself instantly with MSN Messenger! Download today it's FREE!
> > http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
