On 03/03/2015 17:30, Mark Thomas wrote: > On 03/03/2015 15:48, Konstantin Kolinko wrote: >> 2015-03-03 17:47 GMT+03:00 <ma...@apache.org>: >>> Author: markt >>> Date: Tue Mar 3 14:47:12 2015 >>> New Revision: 1663715 >>> >>> URL: http://svn.apache.org/r1663715 >>> Log: >>> In directly fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57251 >> >> You meant "Indirectly". > > I did. > >> 1. The above ordering of setLastModified vs unpacking is wrong. >> >> Adding files to the directory while unpacking the war should change >> its modification time. Your "setLastModified" call will be invalidated >> by later changes. > > I'll check that and fix it. > >> 2. I wonder why I do not see any check for directory time stamp in >> this commit. It only compares time stamps of xml and war files. > > ExpndWar should be correct. HostConfig needs fixing. > >> 3. Generally, I am not sure how well setting modification time of a >> directory works cross-platform. There may be some edge cases here. I >> am more used to file modification times. >> >> Probably modern OSes are OK, but there may be some issues with drives >> accessed over network. (Just a guess. I do not have an actual >> example.) >> >> This can be solved by allowing to opt-out from this feature - see 4. below. > > Absent an actual problem I'd rather not add another configuration option. > > I should be able to test if setting the timestamp works and skip the > test if it doesn't. The only catch is that the code that checks the > timestamp and the code that sets the timestamp are widely separated at > the moment. A little refactoring may be required. > >> 4. If you are going to backport this, > > I intend to backport it to 8 and no further unless there is a demand for > it from the users. > >> I think we need to allow to opt out of last modified check on the >> expanded directory. >> >> Setting the date is OK - we are ignoring the boolean return value of >> that method, checking the date has to be configurable. >> >> Use case: >> The expanded directory is created via some other legacy method. >> >> E.g. by unpacking the war by some configuration tool. There may be >> some legacy scripts doing such unpacking. >> Maybe chef recipes, maybe RPMs. Personally I would just pack an >> expanded directory into the RPM, but I do not know what people will be >> doing. >> >> Someone can do unpacking from a script to save time on subsequent >> Tomcat startup, >> or if the same appbase is shared by several Tomcat instances. >> >> The script may be not smart enough to read timestamp of a file and set >> timestamp on the directory. (As a future help: "touch -r reffile" >> can be used to copy a timestamp from another file). >> >> >> One more example: the war and expanded directory are stored in svn or >> git. Those do not track time stamps of directories. > > Disabling unpackWARs should suffice (or is necessary for correct > operation in some cases) for those use cases. > > The change needs to be clearly documented in the docs, migration guide > and the release announcement.
Hmm. A bigger issue is anything writing files into the docBase (e.g. some people write logs here despite it being a bad idea) is going to change the last modified time of the directory. As much as I like the simplicity of this approach, I think an alternative is required. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
