OFBiz
  1. OFBiz
  2. OFBIZ-4856

webapp "tempfiles" is not successfully loaded because of java.io.FileNotFoundException during startup

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Component/s: framework
    • Labels:
      None

      Description

      During startup, there's a "java.io.FileNotFoundException" thrown out when ofbiz trying to load webapp "tempfiles" which is defined by framework/images component.

      The file not found is "runtime/tempfiles/WEB-INF/web.xml". This file does not exist from the very first, catalina container will use default web.xml instead.

      I apply a patch for this issue to catch the "java.io.FileNotFoundException" and bypass it.

        Activity

        Hide
        Jacopo Cappellato added a comment -

        Unfortunately I didn't have time to review all the work around the "tempfiles" application but I have some doubts about the way it is implemented... in my opinion it should be reviewed carefully.

        Show
        Jacopo Cappellato added a comment - Unfortunately I didn't have time to review all the work around the "tempfiles" application but I have some doubts about the way it is implemented... in my opinion it should be reviewed carefully.
        Hide
        Jacques Le Roux added a comment -

        ANyway Jacopo, this patch can't hurt. I propose to commit it, OK?

        Show
        Jacques Le Roux added a comment - ANyway Jacopo, this patch can't hurt. I propose to commit it, OK?
        Hide
        Jacques Le Roux added a comment -

        Initial commit for the tempfiles stuff was http://svn.apache.org/viewvc?view=revision&revision=736896

        Show
        Jacques Le Roux added a comment - Initial commit for the tempfiles stuff was http://svn.apache.org/viewvc?view=revision&revision=736896
        Hide
        Jacopo Cappellato added a comment -

        What I don't like about committing this fix is that it will fix the error and hide the bad design that was causing it.
        I had a quick look at the revision that Jacques mentioned (thank you) and I have noticed some very bad code:

        http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/Captcha.java?view=markup&pathrev=736896

        this class is not thread safe and if fact I already encountered a bad bug in production because of this; there are also some formatting issues... I understand that this class has been copied from another project (did we mention this in our notice file? I don't remember if it is mandatory but we should check) but rev. 736896 is a perfect example of code that should have never been committed in this way because it is dangerous, it doesn't meet minimal quality requirements.

        Show
        Jacopo Cappellato added a comment - What I don't like about committing this fix is that it will fix the error and hide the bad design that was causing it. I had a quick look at the revision that Jacques mentioned (thank you) and I have noticed some very bad code: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/Captcha.java?view=markup&pathrev=736896 this class is not thread safe and if fact I already encountered a bad bug in production because of this; there are also some formatting issues... I understand that this class has been copied from another project (did we mention this in our notice file? I don't remember if it is mandatory but we should check) but rev. 736896 is a perfect example of code that should have never been committed in this way because it is dangerous, it doesn't meet minimal quality requirements.
        Hide
        Hans Bakker added a comment -

        This is a pretty strong statement about another Apache project....

        Regards,
        Hans

        Show
        Hans Bakker added a comment - This is a pretty strong statement about another Apache project.... Regards, Hans
        Hide
        Jacopo Cappellato added a comment -

        Yep, that code is really awful; I had a look at the source files referenced in Captcha.java:

        http://cocoon.apache.org/2.2/blocks/captcha/1.0/1436_1_1.html

        but I couldn't find that class... is the reference the right one?
        By the way, there are two possibilities:

        • the Cocoon code you have copied is really bad written
        • the Cocoon code was not intended to be used in a multi threaded environment like OFBiz is doing

        In both ways the code as is should have never been committed.
        Hans, apart from commenting my statements, do you have anything to say to the thread unsafety of this class?

        Show
        Jacopo Cappellato added a comment - Yep, that code is really awful; I had a look at the source files referenced in Captcha.java: http://cocoon.apache.org/2.2/blocks/captcha/1.0/1436_1_1.html but I couldn't find that class... is the reference the right one? By the way, there are two possibilities: the Cocoon code you have copied is really bad written the Cocoon code was not intended to be used in a multi threaded environment like OFBiz is doing In both ways the code as is should have never been committed. Hans, apart from commenting my statements, do you have anything to say to the thread unsafety of this class?
        Hide
        Hans Bakker added a comment -

        Jacopo,

        In general when we need an important function in OFBiz which is missing
        like Captcha, we look in an existing Apache project first. If we find
        it, we use it without much checking. I personally do not have enough
        knowledge to provide you with a comment on thread "unsafety."

        Regards,
        Hans

        Show
        Hans Bakker added a comment - Jacopo, In general when we need an important function in OFBiz which is missing like Captcha, we look in an existing Apache project first. If we find it, we use it without much checking. I personally do not have enough knowledge to provide you with a comment on thread "unsafety." Regards, Hans
        Hide
        Jacques Le Roux added a comment -

        Jacopo,

        Just curious, what is the relation between a missing web.xml file an not being thread safe?

        Show
        Jacques Le Roux added a comment - Jacopo, Just curious, what is the relation between a missing web.xml file an not being thread safe?
        Hide
        Jacopo Cappellato added a comment -

        Hans: could you please send me a reference (a URL to the source file in svn would be great) to the Cocoon file that you copied? I would like to review it before taking a decision about Captcha.java.

        Jacques: there is no relation between the patch and the bad code that I am concerned about; the whole design is wrong and we could decide to fix it by refactoring it... at that point the webapp mounted from the runtime folder (?!) may go away, suppressing the error at the root.

        Show
        Jacopo Cappellato added a comment - Hans: could you please send me a reference (a URL to the source file in svn would be great) to the Cocoon file that you copied? I would like to review it before taking a decision about Captcha.java. Jacques: there is no relation between the patch and the bad code that I am concerned about; the whole design is wrong and we could decide to fix it by refactoring it... at that point the webapp mounted from the runtime folder (?!) may go away, suppressing the error at the root.
        Hide
        Jacques Le Roux added a comment -

        Makes sense, thank you

        Show
        Jacques Le Roux added a comment - Makes sense, thank you
        Hide
        Hans Bakker added a comment -

        Sorry Jacopo, that is too far back to know exactly where, at that time
        we got it from, but sure it was an apache project.
        Since you did not agree to it anyway, replace it with whatever you want?

        Regards,
        Hans

        Show
        Hans Bakker added a comment - Sorry Jacopo, that is too far back to know exactly where, at that time we got it from, but sure it was an apache project. Since you did not agree to it anyway, replace it with whatever you want? Regards, Hans
        Hide
        Jacopo Cappellato added a comment -

        I am sorry Hans, but if you have copied code from another project we have to keep track of it and it is not an option to forget about this... this may cause license issues.
        We cannot commit code that is broken or only partially working and then pretend that others will fix it: do you and anyone else understand that this way of doing (at least in the last 2 years) is really unacceptable?
        If you do not know how to implement a thread safety class you should not touch the framework code.
        Questions to all committers: should we revert this work? or are we willing to refactor it to make it really usable? Volunteers?

        Show
        Jacopo Cappellato added a comment - I am sorry Hans, but if you have copied code from another project we have to keep track of it and it is not an option to forget about this... this may cause license issues. We cannot commit code that is broken or only partially working and then pretend that others will fix it: do you and anyone else understand that this way of doing (at least in the last 2 years) is really unacceptable? If you do not know how to implement a thread safety class you should not touch the framework code. Questions to all committers: should we revert this work? or are we willing to refactor it to make it really usable? Volunteers?
        Hide
        Hans Bakker added a comment -

        Jacopo,

        no license issues: the file was copied over as is, including the
        header......

        the fact is: if we cannot copy code from an Apache project before
        completely investigating it...

        let me know, did you investigate the free marker code before including
        the jar?

        Regards,
        Hans

        P.S do not worry, apart from smaller changes I will avoid the framework
        as much as possible and always advertise that we are application
        programmers.

        Show
        Hans Bakker added a comment - Jacopo, no license issues: the file was copied over as is, including the header...... the fact is: if we cannot copy code from an Apache project before completely investigating it... let me know, did you investigate the free marker code before including the jar? Regards, Hans P.S do not worry, apart from smaller changes I will avoid the framework as much as possible and always advertise that we are application programmers.
        Hide
        Jacopo Cappellato added a comment -

        Hans,

        what is the purpose of the code in Captcha.deleteFile?

        public static void deleteFile() {
        if (CAPTCHA_FILE_PATH != null)

        { File file = new File(CAPTCHA_FILE_PATH); file.delete(); }

        }

        If I read it right it deletes the folder runtime/tempfiles/captcha/ rather than an individual file.

        Show
        Jacopo Cappellato added a comment - Hans, what is the purpose of the code in Captcha.deleteFile? public static void deleteFile() { if (CAPTCHA_FILE_PATH != null) { File file = new File(CAPTCHA_FILE_PATH); file.delete(); } } If I read it right it deletes the folder runtime/tempfiles/captcha/ rather than an individual file.
        Hide
        Hans Bakker added a comment -

        Jacopo,

        i have no idea what you want to achieve with your questions. You are
        talking about a commit i did more than 3 years ago which was done by one
        of my people. Since 2005 i am on the project with more than 2000
        commits so in average one per day. I hope you understand that i can just
        guess, just as you, why this was done. Probably this was done as part of
        a housekeeping job to delete all the files which were produced by
        registrations.

        I thought we agreed that nobody owns any code so please if you do not
        like this code go ahead and change it? As I already mentioned we are
        extremely busy and currently we just contribute the parts which
        customers ask us to do and which do not need much extra time.

        Regards,
        Hans

        Show
        Hans Bakker added a comment - Jacopo, i have no idea what you want to achieve with your questions. You are talking about a commit i did more than 3 years ago which was done by one of my people. Since 2005 i am on the project with more than 2000 commits so in average one per day. I hope you understand that i can just guess, just as you, why this was done. Probably this was done as part of a housekeeping job to delete all the files which were produced by registrations. I thought we agreed that nobody owns any code so please if you do not like this code go ahead and change it? As I already mentioned we are extremely busy and currently we just contribute the parts which customers ask us to do and which do not need much extra time. Regards, Hans
        Hide
        Jacopo Cappellato added a comment -

        I was simply trying to get some help as you may know better than me what this code was supposed to do; I didn't want to imply that you own the code, in fact you don't and I am already in the process of removing the problematic/dangerous code and reimplementing a part of it.
        Scott is also refactoring another part of it and then we should be in a better spot.
        Going back to my question and your answer:
        "Probably this was done as part of
        a housekeeping job to delete all the files which were produced by
        registrations."

        I don't think it is the case because that code is executed every time a new captcha image is created.

        Show
        Jacopo Cappellato added a comment - I was simply trying to get some help as you may know better than me what this code was supposed to do; I didn't want to imply that you own the code, in fact you don't and I am already in the process of removing the problematic/dangerous code and reimplementing a part of it. Scott is also refactoring another part of it and then we should be in a better spot. Going back to my question and your answer: "Probably this was done as part of a housekeeping job to delete all the files which were produced by registrations." I don't think it is the case because that code is executed every time a new captcha image is created.
        Hide
        Jacopo Cappellato added a comment -

        Fixed in rev. 1337202

        Show
        Jacopo Cappellato added a comment - Fixed in rev. 1337202
        Hide
        Jacques Le Roux added a comment -

        Jacopo,

        If you get a chance, please mention where it has been backported (and revisions if possible)

        Show
        Jacques Le Roux added a comment - Jacopo, If you get a chance, please mention where it has been backported (and revisions if possible)
        Hide
        Jacopo Cappellato added a comment -

        Full set of revisions for trunk: 1335047, 1337057, 1337058, 1337059, 1337202
        Backported to 12.04 with revisions: 1337259, 1337264, 1337266, 1337267, 1337269
        Backported to 11.04 with revisions: 1337335, 1337337, 1337338, 1337341, 1337345

        Show
        Jacopo Cappellato added a comment - Full set of revisions for trunk: 1335047, 1337057, 1337058, 1337059, 1337202 Backported to 12.04 with revisions: 1337259, 1337264, 1337266, 1337267, 1337269 Backported to 11.04 with revisions: 1337335, 1337337, 1337338, 1337341, 1337345
        Hide
        Jacques Le Roux added a comment -

        Thanks

        Show
        Jacques Le Roux added a comment - Thanks
        Hide
        Hans Bakker added a comment -

        Thank you Jacopo solving this problem.

        Regards,
        Hans

        Show
        Hans Bakker added a comment - Thank you Jacopo solving this problem. Regards, Hans
        Hide
        Jacopo Cappellato added a comment -

        You are welcome Hans; however most of the work was actually done by Scott.

        Show
        Jacopo Cappellato added a comment - You are welcome Hans; however most of the work was actually done by Scott.

          People

          • Assignee:
            Jacopo Cappellato
            Reporter:
            Leon
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development