Frank,
Now I fully understand your problem too. I did not see or somehow
skipped over the part about sending back to the ftp-client that an error
occurred.
Agreed that the FileObject close() is for another conversation, I have
one and use it to handle updates for a virtual file system, nothing to
do with sending back an error to the client though.
Your idea on the Stream monitor via a callback is a good one. I have
used something like this in the past, ie, extending the Stream to get
around limitations or watch for certain conditions.
Since the real issue is getting feedback to the client and not trying to
handle an event locally, it comes back that a change needs to be made in
either the IoUtils.close() method or a different approach to exposing
that an error occurred. Hey wait!, didn't you state that in the
beginning ;-) ...
At a minimum a boolean could be returned indicating whether an error was
detected or not. Perhaps sticking to the existing "failure" flag logic,
if 'failure == true' then an error occurred.
The if() routine would be changed to handle both success [this is what
the current code handles now], and failure. Of course this does ask the
question as to why it was skipped to begin with? Where is that book on
Monty Python style programming, I am sure it had something to do with
the coconuts and herring, or maybe it was the knights.
... back to the subject at-hand, the "if()" routine:
if ( failure ) {
// new code routine, probably send back a 550 message?
} else {
// current code routine
}
This change has to occur in every command that uses a FileObject stream,
not just the STOR command. Oops being ambiguous there, it should be any
command that uses a FileObject OutputStream.
Actually it is good practice [cough cough] to send back all errors to
the client. Sort of like people spending hours trying to figure out why
something is not working, only to find out that they used the kitchen
sink (Exception) instead of catching individual ones. Or they took the
other approach, errors? we don't need no stinkin' try{} catch().
Getting a return from a method in the IoUtils is not a big deal, plenty
of precedence there, getUniqueFile(), getBufferedWriter() and others.
So the existing IoUtil.close(outputStream):
/**
* No exception <code>OutputStream</code> close method.
*/
public final static void close(OutputStream os) {
if (os != null) {
try {
os.close();
} catch (Exception ex) {
}
}
}
would look something like this:
/**
* No exception <code>OutputStream</code> close method.
*/
public final static boolean close(OutputStream os) {
boolean failure = true;
if (os != null) {
try {
os.close();
failure = false;
} catch (IOException ex) {
}
}
return failure;
}
Notice the change from Exception to IOException. The latter is the only
exception thrown by the OutputStream close() method. Or at least that is
what is documented :-O.
Then the next question is what to do with the return boolean. Is it a
good idea to use the existing failure boolean? Its possible that the
data transfer may have completed successfully, but the close failed do
to some other non-data related issue.
finally {
failure = IoUtils.close(outStream);
}
if ( failure ) {
// 550 - your data is hosed, er, an exception was detected ...
} else {
}
Niklas? David? Comments?
---
Andy Thomson
Frank van der Kleij wrote:
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/
