When I saw the update I have noticed that there is a code duplicity. And for
someone like me, there is a code inconsistency. The duplicity should be
removed, the inconsistency may be discussed.
The duplicity: Listeners are stopped twice, for first time in
DefaultFtpServer.stop(), for second time in
DefaultFtpServerContext.dispose().
The inconsistency: As DefaultFtpServer starts listeners and initializes a
ftplet container, it should stop and destroy these as well. My understanding
of ftp server context is that it holds instances to be accessible accross
project but would'n have any application logic inside. Thus,
FtpServerContext.dispose() should free only resources - in our case memory.
Next note is about null checking. The first thing DefaultFtpServer.stop()
does is checking of serverContext if it is null, why to check it again
before dispose() is caled? And before ftpletContainer.destroy() is called,
the ftpletContainer is checked if it is null. But lot of code in project
implicitly expects the ftplet container to be not null (find usages of
FtpServerContext.getFtpletContainer() in your IDE).
My approach can be seen in attached file.
Jiří Kuhn.
2008/12/22 Niklas Gustavsson <nik...@protocol7.com>
> You've convinced me. I just added the change where we always detroy
> the Ftplets on shutdown. Thanks a lot for the motivations!
>
> /niklas
>
> 2008/12/22 Sai Pullabhotla <sai.pullabho...@jmethods.com>:
> > I agree with Jiří that if some one chooses to use a custom Ftplet
> > container, it is their responsibility to implement the appropriate
> > methods (such as init and destroy). All we can do is call the destroy
> > method when we think the server is going down. I don't see any reason
> > why we need to keep track of what Ftplet container is in use and make
> > decisions based on that.
> >
> > Regards,
> > Sai Pullabhotla
> > Phone: (402) 408-5753
> > Fax: (402) 408-6861
> > www.jMethods.com
> >
> >
> > On Mon, Dec 22, 2008 at 1:20 AM, Jiří Kuhn <jiri.k...@clapix.com> wrote:
> >> In my opinion, if we call init(), we should call destroy(). It's clear
> and
> >> logic behaviour. Client may provide an instance of the ftplet container
> and
> >> the server calls methods defined by an interface, why to make a
> difference
> >> if the instance is default one or not. The decision is on a client if
> the
> >> destroy() method should be implemented or empty and the destruction of
> the
> >> provided ftplet container is done somewhere else.
> >>
> >> The interface defines a contract, if the client breaks it, it will face
> the
> >> consequences itself.
> >>
> >> At present, if you use everything default, your ftplets are never
> >> destroyed(). This is wrong, I think.
> >>
> >> Jiří Kuhn.
> >>
> >> 2008/12/19 Niklas Gustavsson <nik...@protocol7.com>
> >>
> >>> On Fri, Dec 19, 2008 at 8:14 AM, Jiří Kuhn <jiri.k...@clapix.com>
> wrote:
> >>> > destroy() method on ftplet container is never called. I gues that
> >>> something
> >>> > like
> >>> >
> >>> > serverContext.getFtpletContainer().destroy();
> >>> >
> >>> > should be placed somewhere inside DefaultFtpServer.stop() method.
> >>> Probably
> >>> > right just before serverContext disposition to be, let's say,
> consistent
> >>> > with init() method. I really wonder that none has noticed it yet. Or
> am I
> >>> > missing something?
> >>>
> >>> I'm aware of this, but unsure how we best handle it. If the user has
> >>> provided their own FtpletContainer (pretty unlikely but possible), I
> >>> think the API client should destroy the container itself. However, in
> >>> most cases, clients will simple add Ftplets to the server, in this
> >>> case we should destroy on shutdown. So, should we keep track of the
> >>> fact that we're using our default provided FtpletContainer and only
> >>> destroy if that's true?
> >>>
> >>> /niklas
> >>>
> >>
> >
>
Index: core/src/main/java/org/apache/ftpserver/impl/DefaultFtpServer.java
===================================================================
--- core/src/main/java/org/apache/ftpserver/impl/DefaultFtpServer.java (revision 728879)
+++ core/src/main/java/org/apache/ftpserver/impl/DefaultFtpServer.java (working copy)
@@ -114,11 +114,12 @@
listener.stop();
}
+ // destroy the Ftplet container
+ serverContext.getFtpletContainer().destroy();
+
// release server resources
- if (serverContext != null) {
- serverContext.dispose();
- serverContext = null;
- }
+ serverContext.dispose();
+ serverContext = null;
started = false;
}
Index: core/src/main/java/org/apache/ftpserver/impl/DefaultFtpServerContext.java
===================================================================
--- core/src/main/java/org/apache/ftpserver/impl/DefaultFtpServerContext.java (revision 728879)
+++ core/src/main/java/org/apache/ftpserver/impl/DefaultFtpServerContext.java (working copy)
@@ -189,17 +189,8 @@
* Close all the components.
*/
public void dispose() {
-
- Iterator<Listener> listenerIter = listeners.values().iterator();
- while (listenerIter.hasNext()) {
- Listener listener = listenerIter.next();
- listener.stop();
- }
-
- // now tell Ftplets to destroy themselves
- if(ftpletContainer != null) {
- ftpletContainer.destroy();
- }
+ listeners.clear();
+ ftpletContainer.getFtplets().clear();
}
public Listener getListener(String name) {
Index: core/src/main/java/org/apache/ftpserver/ftpletcontainer/impl/DefaultFtpletContainer.java
===================================================================
--- core/src/main/java/org/apache/ftpserver/ftpletcontainer/impl/DefaultFtpletContainer.java (revision 728879)
+++ core/src/main/java/org/apache/ftpserver/ftpletcontainer/impl/DefaultFtpletContainer.java (working copy)
@@ -94,7 +94,6 @@
LOG.error(entry.getKey() + " :: FtpletHandler.destroy()", ex);
}
}
- ftplets.clear();
}
/**
