Commons IO
  1. Commons IO
  2. IO-116

Replace static FileCleaner methods

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.3.1
    • Fix Version/s: 1.3.2
    • Component/s: Utilities
    • Labels:
      None

      Description

      The attached patch aims to finally resolve the problems, which are named in IO-99, FILEUPLOAD-120, and FILEUPLOAD-125.

      I choosed a conservative strategy: Basically I copied the FileCleaner class to an instantiable class FileCleaningTracker with instance methods. The static FileCleaner methods are now implemented by a static instance of FileCleaningTracker. (The name FileCleaningTracker is, of course, questionable.

      The FileCleaningTestCase was also created by simply copying FileCleaner to FileCleaningTestCase. FileCleanerTestCase is now similarly implemented as a subclass of FileCleanerTestCase which uses the static instance of FileCleaner rather than a dynamically created instance.

      1. commons-io-filecleaningtracker.patch
        43 kB
        Jochen Wiedmann
      2. commons-io-filecleaningtracker.patch
        43 kB
        Jochen Wiedmann

        Issue Links

          Activity

          Hide
          Holger Hoffstätte added a comment -

          Hi Jochen, this is already much better than what is in the last release but please check out my suggestion to a similar problem in LANG-324. Your patch still creates the reaper Thread and it would be IMHO better to let the user/webapp/environment manage the lifecycle of that Thread, if only to be able to properly interrupt it.
          Also instead of using an extra Thread it should be possible to use an injected Timer; deleting the files is (as far as I understand it?) not a real-time activity since it happens asynchronously anyway.
          Creating/keeping static Thread references should have never made it into a released commons API

          Show
          Holger Hoffstätte added a comment - Hi Jochen, this is already much better than what is in the last release but please check out my suggestion to a similar problem in LANG-324 . Your patch still creates the reaper Thread and it would be IMHO better to let the user/webapp/environment manage the lifecycle of that Thread, if only to be able to properly interrupt it. Also instead of using an extra Thread it should be possible to use an injected Timer; deleting the files is (as far as I understand it?) not a real-time activity since it happens asynchronously anyway. Creating/keeping static Thread references should have never made it into a released commons API
          Hide
          Jochen Wiedmann added a comment -

          Holger, I may get things wrong, but IMO you are perfectly capable of controlling the threads lifetime:

          • You create your own instance of FileCleaningTracker.
          • The thread is started whenever the first file is added to the instance as trackable.
          • The thread may be terminated by invoking exitWhenFinished().

          For a new lifecycle, simply create a new instance. So, what's wrong in your opinion?

          Show
          Jochen Wiedmann added a comment - Holger, I may get things wrong, but IMO you are perfectly capable of controlling the threads lifetime: You create your own instance of FileCleaningTracker. The thread is started whenever the first file is added to the instance as trackable. The thread may be terminated by invoking exitWhenFinished(). For a new lifecycle, simply create a new instance. So, what's wrong in your opinion?
          Hide
          Holger Hoffstätte added a comment -

          Jochen, yes you are right: it is possible to control the lifetime & shutdown properly but I was suggesting that we prevent the creation of yet another extra Thread at all. Active objects don't really need their own Thread, they only need to be run in certain intervals and decoupled. If every little object would start its own Thread all hell would break loose. That was all, really.
          (Java really needs a better abstraction for lightweight active objects that do not correspond 1:1 to Threads..)

          Show
          Holger Hoffstätte added a comment - Jochen, yes you are right: it is possible to control the lifetime & shutdown properly but I was suggesting that we prevent the creation of yet another extra Thread at all. Active objects don't really need their own Thread, they only need to be run in certain intervals and decoupled. If every little object would start its own Thread all hell would break loose. That was all, really. (Java really needs a better abstraction for lightweight active objects that do not correspond 1:1 to Threads..)
          Hide
          Jochen Wiedmann added a comment -

          Holger, of course there is no such thing as a thread for "every little object". The thread is started when the first object is attached to the instance. Typically, you would create the instance in your web applications init method and call exitWhenFinished in the webapps destroy method. That's all there is.

          Show
          Jochen Wiedmann added a comment - Holger, of course there is no such thing as a thread for "every little object". The thread is started when the first object is attached to the instance. Typically, you would create the instance in your web applications init method and call exitWhenFinished in the webapps destroy method. That's all there is.
          Hide
          Holger Hoffstätte added a comment -

          Hi,

          ich fürchte wir reden total aneinander vorbei

          Ich weiß schon wie der FileTracker funktioniert, und auch daß er als
          "Dienst" eine etwas andere Rolle spielen darf als z.B. die üblichen
          statischen Helfer-Methoden. Gerade aber weil er nur ab und zu laufen muß,
          ist es ein guter Kandidat für einen Timer - weil z.B. eine Webapp ja auch
          garantiert noch andere Dinge zu tun hat, und es wäre sicherlich einfacher,
          nur einen Timer pro Webapp für alle periodischen Dinge zu benutzen/zu
          kontrollieren als x verschiedene Mechanismen.

          Worauf ich hinauswollte: Bibliotheken (im Gegensatz zu Frameworks) "an
          sich" sollten eben überhaupt gar keine Threads starten - weil das sonst
          eben genau darauf hinausläuft, daß jede Klasse ihr Thread-Handling selbst
          neu stricken muß, mit ClassLoader-leaks und dem ganzen Klimbim.

          Es gibt unzählige Verwendungen für asynchrone Objekte in Collections, IO,
          Lang, Pool, DBCP usw. und obwohl eigentlich nur 1-2 gebraucht würden um
          die Arbeit zu erledigen hätte man am Ende mehrere hundert Threads laufen,
          nur weil wieder keiner über APIs und Komposition/Zusammenspiel im größeren
          Rahmen nachgedacht hat. Wenn ich in meiner App z.B. 100 kleine Pools habe,
          die sich alle 5 Minuten periodisch selbst säubern können, brauche ich
          dafür wirklich keine 100 Threads, die 99.99% der Zeit einfach nichts tun.

          Mein Kommentar war lediglich als kleiner Denkanstoß für commons allgemein
          gedacht.

          ciao
          Holger

          Show
          Holger Hoffstätte added a comment - Hi, ich fürchte wir reden total aneinander vorbei Ich weiß schon wie der FileTracker funktioniert, und auch daß er als "Dienst" eine etwas andere Rolle spielen darf als z.B. die üblichen statischen Helfer-Methoden. Gerade aber weil er nur ab und zu laufen muß, ist es ein guter Kandidat für einen Timer - weil z.B. eine Webapp ja auch garantiert noch andere Dinge zu tun hat, und es wäre sicherlich einfacher, nur einen Timer pro Webapp für alle periodischen Dinge zu benutzen/zu kontrollieren als x verschiedene Mechanismen. Worauf ich hinauswollte: Bibliotheken (im Gegensatz zu Frameworks) "an sich" sollten eben überhaupt gar keine Threads starten - weil das sonst eben genau darauf hinausläuft, daß jede Klasse ihr Thread-Handling selbst neu stricken muß, mit ClassLoader-leaks und dem ganzen Klimbim. Es gibt unzählige Verwendungen für asynchrone Objekte in Collections, IO, Lang, Pool, DBCP usw. und obwohl eigentlich nur 1-2 gebraucht würden um die Arbeit zu erledigen hätte man am Ende mehrere hundert Threads laufen, nur weil wieder keiner über APIs und Komposition/Zusammenspiel im größeren Rahmen nachgedacht hat. Wenn ich in meiner App z.B. 100 kleine Pools habe, die sich alle 5 Minuten periodisch selbst säubern können, brauche ich dafür wirklich keine 100 Threads, die 99.99% der Zeit einfach nichts tun. Mein Kommentar war lediglich als kleiner Denkanstoß für commons allgemein gedacht. ciao Holger
          Hide
          Holger Hoffstätte added a comment -

          sigh - that last comment was meant as private email. please ignore.

          Show
          Holger Hoffstätte added a comment - sigh - that last comment was meant as private email. please ignore.
          Hide
          Jochen Wiedmann added a comment -

          Holger, if I get you right, what you intend to tell is this:

          There are a lot of places in the various commons-foo libraries, which create/destroy/manage threads. Your recommendation is to have a management framework, where these threads can be integrated, the target being to reduce the number of required threads.

          Depending on the application (Mule in your case?), that seems a sensible request. However, I do believe that this is clearly beyond the scope of this issue, which simply wants to create the possibility to have a custom lifecycle at all. The question how this lifecycle is managed can clearly be resolved later.

          I must admit, that I have no idea how your concerns might be approached. I do not even know, whether there already is an existing framework, which might be reused for that. (Quartz, may be, but it seems the scope of Quartz is more related to the end user and not the framework developer.) Perhaps, your ideas are sufficient for introducing a new component in the sandbox. However, as long as there is not even an API for such a component, I see no reason to address the integration into it here.

          Show
          Jochen Wiedmann added a comment - Holger, if I get you right, what you intend to tell is this: There are a lot of places in the various commons-foo libraries, which create/destroy/manage threads. Your recommendation is to have a management framework, where these threads can be integrated, the target being to reduce the number of required threads. Depending on the application (Mule in your case?), that seems a sensible request. However, I do believe that this is clearly beyond the scope of this issue, which simply wants to create the possibility to have a custom lifecycle at all. The question how this lifecycle is managed can clearly be resolved later. I must admit, that I have no idea how your concerns might be approached. I do not even know, whether there already is an existing framework, which might be reused for that. (Quartz, may be, but it seems the scope of Quartz is more related to the end user and not the framework developer.) Perhaps, your ideas are sufficient for introducing a new component in the sandbox. However, as long as there is not even an API for such a component, I see no reason to address the integration into it here.
          Hide
          Henri Yandell added a comment -

          And the big problem with adding a framework for this is that it's better to have a Thread sitting in IO, Lang etc than to add a dependency to them for a small part of the feature-set.

          Show
          Henri Yandell added a comment - And the big problem with adding a framework for this is that it's better to have a Thread sitting in IO, Lang etc than to add a dependency to them for a small part of the feature-set.
          Hide
          Holger Hoffstätte added a comment -

          I did NOT want to add a "framework", but rather exactly the opposite.

          Show
          Holger Hoffstätte added a comment - I did NOT want to add a "framework", but rather exactly the opposite.
          Hide
          Jochen Wiedmann added a comment -

          I must admit, that I can't follow you, Holger. Nevertheless, I suggest
          that we move the discussion to the mailing list, because my impression
          is still, that it's off-topic in IO-116. Fix me, if I'm wrong.

          Jochen


          Emacs 22 will support MacOS and CygWin. It is not yet decided, whether
          these will be used to run Emacs or the other way round.

          Show
          Jochen Wiedmann added a comment - I must admit, that I can't follow you, Holger. Nevertheless, I suggest that we move the discussion to the mailing list, because my impression is still, that it's off-topic in IO-116 . Fix me, if I'm wrong. Jochen – Emacs 22 will support MacOS and CygWin. It is not yet decided, whether these will be used to run Emacs or the other way round.
          Hide
          Jochen Wiedmann added a comment -

          Same patch, next attempt

          Show
          Jochen Wiedmann added a comment - Same patch, next attempt
          Hide
          Henri Yandell added a comment -

          As the patch doesn't apply cleanly for me - +1 on applying it (you have karma after all).

          Show
          Henri Yandell added a comment - As the patch doesn't apply cleanly for me - +1 on applying it (you have karma after all).
          Hide
          Henri Yandell added a comment -

          Some minor comments (they always occur after you say +1 for some reason):

          • Javadoc on FileCleaningTestCase refers to FileCleaner.
          • Given that FileCleaner is static, why not implement FileCleaningTestCase inside FileCleaner?

          ie) deprecate the static methods and add non-static methods etc. I know that BeanUtils didn't do this, so I'm guessing there's some kind of reason for that.

          Show
          Henri Yandell added a comment - Some minor comments (they always occur after you say +1 for some reason): Javadoc on FileCleaningTestCase refers to FileCleaner. Given that FileCleaner is static, why not implement FileCleaningTestCase inside FileCleaner? ie) deprecate the static methods and add non-static methods etc. I know that BeanUtils didn't do this, so I'm guessing there's some kind of reason for that.
          Hide
          Jochen Wiedmann added a comment -

          Applied. Henri, I did not pick up your suggestion to put the test case into FileCleaner itself. It seems to be to unusual to me and personally I do prefer a clean separation between test and tested code. If anyone starts this style elsewhere, I am ready to change that later.

          Show
          Jochen Wiedmann added a comment - Applied. Henri, I did not pick up your suggestion to put the test case into FileCleaner itself. It seems to be to unusual to me and personally I do prefer a clean separation between test and tested code. If anyone starts this style elsewhere, I am ready to change that later.
          Hide
          Henri Yandell added a comment -

          Agreed - that would be mad I think I meant:

          • Given that FileCleaner is static, why not implement FileCleaningTestCase inside FileCleanerTestCase?

          Or rather:

          Why have a FileCleanerTestCase?

          Will bring up on list.

          Show
          Henri Yandell added a comment - Agreed - that would be mad I think I meant: Given that FileCleaner is static, why not implement FileCleaningTestCase inside FileCleanerTestCase? Or rather: Why have a FileCleanerTestCase? Will bring up on list.

            People

            • Assignee:
              Unassigned
              Reporter:
              Jochen Wiedmann
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development