I'll merge this tomorrow if nobody objects. Changes: * Move EnvironmentConfiguration into /internal/, it's not API for public consumption. * New module commons-storage, contains nothing other than UnexpectedException, other things may be moved layer. * Temporary files are now in xwiki-temp subdirectory which is deleted on start-up. * Slightly more stable behavior, if a writable temp/persistent dir is not available, an UnexpectedException (RuntimeException) is thrown rather than giving the caller an unwritable directory.
https://github.com/xwiki/xwiki-commons/compare/XWIKI-7748 Caleb On 05/02/2012 11:39 AM, Caleb James DeLisle wrote: > Lets try this again... > > Replaced the poorly thought out exit() call with a throw of an unchecked > exception. > The unchecked exception which I usually use to signal that the storage is > broken due > to misconfiguration or malfunction is the UnexpectedException. > In order to use this in commons, I created commons-store, which now only > contains that > one exception but in the future might contain other classes which are battle > tested, > have already gone through their refactorings, and have few or no dependencies. > > https://github.com/xwiki/xwiki-commons/commit/26c714ce33a78fe02546bb1137f4fc5a389a070a > > WDYT? > > Caleb > > > On 05/02/2012 10:09 AM, Caleb James DeLisle wrote: >> >> >> On 05/02/2012 09:49 AM, Vincent Massol wrote: >>> >>> On May 2, 2012, at 3:55 PM, Caleb James DeLisle wrote: >>> >>>> >>>> >>>> On 05/02/2012 09:20 AM, Vincent Massol wrote: >>>>> Hi Caleb, >>>>> >>>>> On May 2, 2012, at 3:12 PM, Caleb James DeLisle wrote: >>>>> >>>>>> Just committed the patch to a branch, you can see it here >>>>>> https://github.com/xwiki/xwiki-commons/commit/32fb77049fe9d8f758e776d26bfe5d7d2d21c4cc >>>>>> >>>>>> Since it contains an API break in EnvironmentConfiguration, I wanted to >>>>>> run it by the list before merging it into the master. >>>>>> I'll push later on today if nobody objects. >>>>> >>>>> We should no longer break APIs. So this will need to go through >>>>> deprecation and legacy. >>>>> >>>>> Why is File an issue? >>>>> >>>>> Big -1 to the code I've seen. We should never ever use System.exit(). >>>> >>>> How do you envision the wiki behaving as a whole if there is no writable >>>> temp dir? >>>> I agree that killing the jvm is very bad if there is any way to recover >>>> but this seems like the edge case where there is no way out. >>> >>> There's no reason to kill the JVM IMO. >>> >>> The only thing we should do is to not make the XWiki webapp available in >>> the running container. This is done by throwing a top level exception. >>> >>> Remember that a servlet container can be used to run several webapps, not >>> just xwiki. Killing it is really really bad. >> >> On second thought I agree since all other "it's dead" errors such as >> database connection stuff results in an error page for the http request. >> Thanks for calling me out. >> >> Caleb >> >>> >>> Thanks >>> -Vincent >>> >>>>> Also I've seen a lot of "final" used in the code. Please don't use them >>>>> since they're unnecessary and bad in most cases and we don't have any >>>>> best practice to use them in methods for ex (this prevents >>>>> extensibility). As for variables I don't see the point at all. >>>> >>>> I can remove the final annotation on the methods, I think it's important >>>> however to differentiate between clean extensibility and using "extends" >>>> to monkey patch code in ways that weren't intended. >>>> As for variables and fields, I am very fond of making those final because >>>> they are inline documentation and verification. >>>> I declare my intent not to change a variable with the final keyword, the >>>> reader/maintainer knows what I meant and the compiler checks that I didn't >>>> make a mistake. >>>> If you want me to strip those, I can but think it's like stripping >>>> comments. >>>> >>>> Caleb >>>> >>>>> >>>>> Re the fail fast I was expecting some event listener listening on >>>>> Application started event to check if the directories were writable for >>>>> example and throw some exception if not. >>>>> >>>>> Thanks >>>>> -Vincent >>>>> >>>>>> Caleb >>>>>> >>>>>> >>>>>> On 04/23/2012 09:43 AM, Caleb James DeLisle wrote: >>>>>>> Hi, >>>>>>> >>>>>>> In order to fix XWIKI-7748 and XWIKI-7749, we need a plan for deleting >>>>>>> temporary file. >>>>>>> XWikiAttachmentContent uses File.deleteOnExit(), but in Sun's >>>>>>> implementation, deleteOnExit() >>>>>>> only deletes on a *clean* exit, in a crash it can't delete the files >>>>>>> but it doesn't tag them >>>>>>> so they can be deleted in the next run either. Since temp files are >>>>>>> usually pseudo-randomly >>>>>>> named, they are impossible to find and delete later. >>>>>>> >>>>>>> I propose that we create a subdirectory, `java.io.tmpdir`/xwiki/ which >>>>>>> will be removed on >>>>>>> application exit. If the jvm crashes, the files will be removed next >>>>>>> time around. Then alter >>>>>>> ApplicationContext.getTemporaryDirectory() to yield this directory. >>>>>>> >>>>>>> WDYT? >>>>>>> >>>>>>> Caleb >>>>> _______________________________________________ >>>>> devs mailing list >>>>> [email protected] >>>>> http://lists.xwiki.org/mailman/listinfo/devs >>>>> >>>> >>>> _______________________________________________ >>>> devs mailing list >>>> [email protected] >>>> http://lists.xwiki.org/mailman/listinfo/devs >>> >>> _______________________________________________ >>> devs mailing list >>> [email protected] >>> http://lists.xwiki.org/mailman/listinfo/devs >>> >> >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs >> > > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
