Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
вт, 18 сент. 2018 г. в 12:39, Mark Thomas : > > On 17/09/18 22:13, Christopher Schultz wrote: > > Mark, > > > > On 9/17/18 08:34, Mark Thomas wrote: > >> On 17/09/18 10:50, Konstantin Kolinko wrote: > > > >> > > > >>> Implementing auto-reloading has a caveat: there is a race > >>> condition between an editor (that is used to update the file) and > >>> Tomcat. It may be that Tomcat will try to read an incompletely > >>> written file. > >>> > >>> Also using a SAX parser + Digester, it does not check whether the > >>> XML file is well-formed beforehand. It stops on the first > >>> encountered error, but side effect from whatever methods it has > >>> called thus far will be visible. > > > >> There is some code that handles a similar case for updated WAR > >> files. We should be able to make that generic and re-use it here. > > > > Also, when using SAX+digester, a new object should be created for the > > user database and only brought into service when it's been completely > > (and properly) loaded. I wouldn't suggest reading the XML directly > > into the live structure. > > > > I'm not saying that's how the code IS written... just saying how it > > ought to be written, given your concerns for such a race-condition. > > Ideally, yes. Unfortunately that would require some additional > refactoring. I think I'll go for the simpler option of clearing the > users, roles and groups if there is an error. The lock that is now in > place will ensure the data isn't used during the load. Good point: there will be no bogus data in the hashmaps. Bad point: if reload was triggered via JMX proxy servlet, clearing the hashmaps means that it won't be possible to trigger the reload anymore, as the manager app user will be removed from the table. Best regards, Konstantin Kolinko - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
On 17/09/18 22:13, Christopher Schultz wrote: > Mark, > > On 9/17/18 08:34, Mark Thomas wrote: >> On 17/09/18 10:50, Konstantin Kolinko wrote: > >> > >>> Implementing auto-reloading has a caveat: there is a race >>> condition between an editor (that is used to update the file) and >>> Tomcat. It may be that Tomcat will try to read an incompletely >>> written file. >>> >>> Also using a SAX parser + Digester, it does not check whether the >>> XML file is well-formed beforehand. It stops on the first >>> encountered error, but side effect from whatever methods it has >>> called thus far will be visible. > >> There is some code that handles a similar case for updated WAR >> files. We should be able to make that generic and re-use it here. > > Also, when using SAX+digester, a new object should be created for the > user database and only brought into service when it's been completely > (and properly) loaded. I wouldn't suggest reading the XML directly > into the live structure. > > I'm not saying that's how the code IS written... just saying how it > ought to be written, given your concerns for such a race-condition. Ideally, yes. Unfortunately that would require some additional refactoring. I think I'll go for the simpler option of clearing the users, roles and groups if there is an error. The lock that is now in place will ensure the data isn't used during the load. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Mark, On 9/17/18 08:34, Mark Thomas wrote: > On 17/09/18 10:50, Konstantin Kolinko wrote: > > > >> Implementing auto-reloading has a caveat: there is a race >> condition between an editor (that is used to update the file) and >> Tomcat. It may be that Tomcat will try to read an incompletely >> written file. >> >> Also using a SAX parser + Digester, it does not check whether the >> XML file is well-formed beforehand. It stops on the first >> encountered error, but side effect from whatever methods it has >> called thus far will be visible. > > There is some code that handles a similar case for updated WAR > files. We should be able to make that generic and re-use it here. Also, when using SAX+digester, a new object should be created for the user database and only brought into service when it's been completely (and properly) loaded. I wouldn't suggest reading the XML directly into the live structure. I'm not saying that's how the code IS written... just saying how it ought to be written, given your concerns for such a race-condition. - -chris -BEGIN PGP SIGNATURE- Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/ iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlugGOoACgkQHPApP6U8 pFgjCg//acpRwULa57LwxCROI6SaM40T7gfIyiRnF4ZAwkmnIjXltBEd5m6hveBI Ql3BKqzaiJxRD0UMZDFSpFSql+lDZpAIHJfQ5n5E11Q5RWa1V09HkLLt47QqF5F6 K4PoQG+C0YN9E2qBs3XEelsrS+19h3bE3B/FiXyxcOmmSrmUQdnrsCzChmOm7JQp t5iNidwbnEOBxOD82UhQdCPZ2SbZ4bne7Oot0QnGocT0EZe4ePk8YxNxAI0vMc7P amKnME+1bIvoK/mDH/LsFxyjMXbLo6TIGTfaPSNUE8EdSBKWujbFxJz/DgTpIHht p5V/CNyOMMogYOBXw9U8UgzEW/QtEpd1n1glg4e0ysUm7kCjQ/z0crnBYu3beBPx cKzE2P0U7rKioFGtZ9YuuwVZsUfJrBeg9DA5Gf81B3NS4bqCJwmsoXOYqeyP8S8W BxYI/wp+rdSfZLnp20ccXzDQDlpKlEIQukM6doo6g/ITejspnoiH4CLHifmNGDdb WSYQ88lQgbtGG0vkaldNCJX+Eptcuky9myzlO5LYA7Yr7CQsZjW5BYZCgE0vdqH6 HZOnskEFkidrNrZFVdob02xtATMlHb5Le5u1m90xUO6hkngxigkUpF+iJiAefKbW 39IjUuETaghC7t8g5QVs7XnCg/WRBO3YuFACTKeDoeZ8mhaEYnA= =aMtP -END PGP SIGNATURE- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
On 17/09/18 10:50, Konstantin Kolinko wrote: > Implementing auto-reloading has a caveat: there is a race condition > between an editor (that is used to update the file) and Tomcat. It may > be that Tomcat will try to read an incompletely written file. > > Also using a SAX parser + Digester, it does not check whether the XML > file is well-formed beforehand. It stops on the first encountered > error, but side effect from whatever methods it has called thus far > will be visible. There is some code that handles a similar case for updated WAR files. We should be able to make that generic and re-use it here. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
вс, 16 сент. 2018 г. в 23:20, Christopher Schultz : > > On 9/14/18 05:21, Mark Thomas wrote: > > On 14/09/18 10:07, Rémy Maucherat wrote: > >> On Fri, Sep 14, 2018 at 10:41 AM wrote: > >> > >>> Author: markt Date: Fri Sep 14 08:41:02 2018 New Revision: > >>> 1840901 > >>> > >>> URL: http://svn.apache.org/viewvc?rev=1840901=rev Log: > >>> Review thread-safety Document locking strategy Fix a few > >>> issues: - Iterators could throw > >>> ConcurrentModificationException - Syncs on open/save didn't > >>> lock roles Map Update with a view to supporting runtime > >>> reloading (BZ 58590) > >>> > >> > >> This user db stuff was introduced in 4.1 and was never really > >> updated since, nor were other implementations done. It survived > >> because of the manager/jmx management it had. Do you think it > >> would be useful to take it somewhere now, or is this only for > >> 58590 ? > > > > My primary motivation is 58590. I've lost track of the number of > > times I've started Tomcat do test something, only to have to > > restart it to pickup changes to tomcat-users.xml. Of course, fixing > > that means reloading has to be enabled by default. I think that is > > OK but what does everyone else think? > > Having to bounce the app server to adjust the authentication database > is really bad IMHO. Auto-reload isn't necessary, but the fact that a > reload is POSSIBLE is nice. Triggering re-loading via JMX would be > sufficient for me. Implementing auto-reloading has a caveat: there is a race condition between an editor (that is used to update the file) and Tomcat. It may be that Tomcat will try to read an incompletely written file. Also using a SAX parser + Digester, it does not check whether the XML file is well-formed beforehand. It stops on the first encountered error, but side effect from whatever methods it has called thus far will be visible. Best regards, Konstantin Kolinko - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
On 16/09/18 21:20, Christopher Schultz wrote: > Mark, > > On 9/14/18 05:21, Mark Thomas wrote: >> On 14/09/18 10:07, Rémy Maucherat wrote: >>> On Fri, Sep 14, 2018 at 10:41 AM wrote: >>> Author: markt Date: Fri Sep 14 08:41:02 2018 New Revision: 1840901 URL: http://svn.apache.org/viewvc?rev=1840901=rev Log: Review thread-safety Document locking strategy Fix a few issues: - Iterators could throw ConcurrentModificationException - Syncs on open/save didn't lock roles Map Update with a view to supporting runtime reloading (BZ 58590) >>> >>> This user db stuff was introduced in 4.1 and was never really >>> updated since, nor were other implementations done. It survived >>> because of the manager/jmx management it had. Do you think it >>> would be useful to take it somewhere now, or is this only for >>> 58590 ? > >> My primary motivation is 58590. I've lost track of the number of >> times I've started Tomcat do test something, only to have to >> restart it to pickup changes to tomcat-users.xml. Of course, fixing >> that means reloading has to be enabled by default. I think that is >> OK but what does everyone else think? > > Having to bounce the app server to adjust the authentication database > is really bad IMHO. Auto-reload isn't necessary, but the fact that a > reload is POSSIBLE is nice. Triggering re-loading via JMX would be > sufficient for me. We can probably add something to do that too (if it isn't already there - I need to check which methods are exposed). >> While I researched the fix for BZ 58590, I could see various >> thread-safety issues that I thought were worth fixing as problems >> are more likely if a background thread is reloading the user >> database. > >> There were other issues I didn't fix. Concurrent modification may >> leave the db in an inconsistent state. E.g a user with a role that >> has been deleted. It is also possible to add a role to a user that >> is not in the database. > >> I thought about a wider clean-up but it looking like a 'proper' >> solution that addressed all the various edge cases was heading >> towards implementing an in memory RDBMS and that seemed like >> overkill for the use case. > > Even Derby/Javadb/whatever-the-name is also fairly heavy-handed. We > don't need full ACID for what amounts to a HashMap. > >> I think there are some users who maintain the database via JMX so >> I think it is worth keeping. But maybe they only do that because >> the file can't be reloaded. It might be worth a discussion on the >> users list. > > Persistence via JMX is problematic. Reloading the user database from > the disk is preferable if you ask me. Just like being able to trigger > a reload of the TLS key store(s) and trust store(s). Ack. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Mark, On 9/14/18 05:21, Mark Thomas wrote: > On 14/09/18 10:07, Rémy Maucherat wrote: >> On Fri, Sep 14, 2018 at 10:41 AM wrote: >> >>> Author: markt Date: Fri Sep 14 08:41:02 2018 New Revision: >>> 1840901 >>> >>> URL: http://svn.apache.org/viewvc?rev=1840901=rev Log: >>> Review thread-safety Document locking strategy Fix a few >>> issues: - Iterators could throw >>> ConcurrentModificationException - Syncs on open/save didn't >>> lock roles Map Update with a view to supporting runtime >>> reloading (BZ 58590) >>> >> >> This user db stuff was introduced in 4.1 and was never really >> updated since, nor were other implementations done. It survived >> because of the manager/jmx management it had. Do you think it >> would be useful to take it somewhere now, or is this only for >> 58590 ? > > My primary motivation is 58590. I've lost track of the number of > times I've started Tomcat do test something, only to have to > restart it to pickup changes to tomcat-users.xml. Of course, fixing > that means reloading has to be enabled by default. I think that is > OK but what does everyone else think? Having to bounce the app server to adjust the authentication database is really bad IMHO. Auto-reload isn't necessary, but the fact that a reload is POSSIBLE is nice. Triggering re-loading via JMX would be sufficient for me. > While I researched the fix for BZ 58590, I could see various > thread-safety issues that I thought were worth fixing as problems > are more likely if a background thread is reloading the user > database. > > There were other issues I didn't fix. Concurrent modification may > leave the db in an inconsistent state. E.g a user with a role that > has been deleted. It is also possible to add a role to a user that > is not in the database. > > I thought about a wider clean-up but it looking like a 'proper' > solution that addressed all the various edge cases was heading > towards implementing an in memory RDBMS and that seemed like > overkill for the use case. Even Derby/Javadb/whatever-the-name is also fairly heavy-handed. We don't need full ACID for what amounts to a HashMap. > I think there are some users who maintain the database via JMX so > I think it is worth keeping. But maybe they only do that because > the file can't be reloaded. It might be worth a discussion on the > users list. Persistence via JMX is problematic. Reloading the user database from the disk is preferable if you ask me. Just like being able to trigger a reload of the TLS key store(s) and trust store(s). - -chris -BEGIN PGP SIGNATURE- Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/ iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlueuxkACgkQHPApP6U8 pFj38A//T/t1pjSEvznyzAKfY5rtPEwww7a6scMQgPSG+e9DuKDjqzJczAwtPCA1 dN7Ojbt9HkC8n8lpfc8YkVJpADvhze34aJxdg+Fty8aNSr3YIWQmyONjQN/R8eSx Src7NmNGN4Gh3tfIoIAxSNRpvcH+13fkdcWDbwWvVlzO6/QSEkjTtk9EMQ7MUQfd xlyNxrChuEYDCxG6RePqusEc3BuUJktcVl6KGcRcDaoBR/K+AdU2ui7S+3osgE5Y 7+Dziy6Hr8V/abAAB4ov7m6nORXLk+qKuzvaSOpqk8Vhg5yolVrpwTNPROLF5P1J Xk7jBqe6iYR/yvU4mG6Yd3fzcbZME7F7uVk4SBWDikoxk7ZycWyvdr4VzkKe01v+ k1GivUMgLeHMCUqDQlBV1DLlJBEJKtn/VKPa+OSmP0mIJjkC/TMqFrNVWc5Rqho7 0xOloi8MtHjiuXI/tK5UbS0cJf/x8amXJbHTyKSaGNWoSV2G1eMSmuQIhUcx3wHX XjtzQBrvsCsVG9bJe3tcBZMXoa57PIw1z48NkBRQXRAOAtrYGKdOMDchfapXvSUU ugH61FDCwy9k+ZH6KgLFEpsrGIxxJ2dF5qCEt3JbgQcfisnU8QWpTekZu/0/TlJB TgrqfVgApFc+RwQhP72pX4yUCsbuDFm//HoLvslEePv9S5scg70= =8mDs -END PGP SIGNATURE- - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
On 14/09/18 10:07, Rémy Maucherat wrote: > On Fri, Sep 14, 2018 at 10:41 AM wrote: > >> Author: markt >> Date: Fri Sep 14 08:41:02 2018 >> New Revision: 1840901 >> >> URL: http://svn.apache.org/viewvc?rev=1840901=rev >> Log: >> Review thread-safety >> Document locking strategy >> Fix a few issues: >> - Iterators could throw ConcurrentModificationException >> - Syncs on open/save didn't lock roles Map >> Update with a view to supporting runtime reloading (BZ 58590) >> > > This user db stuff was introduced in 4.1 and was never really updated > since, nor were other implementations done. It survived because of the > manager/jmx management it had. Do you think it would be useful to take it > somewhere now, or is this only for 58590 ? My primary motivation is 58590. I've lost track of the number of times I've started Tomcat do test something, only to have to restart it to pickup changes to tomcat-users.xml. Of course, fixing that means reloading has to be enabled by default. I think that is OK but what does everyone else think? While I researched the fix for BZ 58590, I could see various thread-safety issues that I thought were worth fixing as problems are more likely if a background thread is reloading the user database. There were other issues I didn't fix. Concurrent modification may leave the db in an inconsistent state. E.g a user with a role that has been deleted. It is also possible to add a role to a user that is not in the database. I thought about a wider clean-up but it looking like a 'proper' solution that addressed all the various edge cases was heading towards implementing an in memory RDBMS and that seemed like overkill for the use case. I think there are some users who maintain the database via JMX so I think it is worth keeping. But maybe they only do that because the file can't be reloaded. It might be worth a discussion on the users list. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1840901 - /tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
On Fri, Sep 14, 2018 at 10:41 AM wrote: > Author: markt > Date: Fri Sep 14 08:41:02 2018 > New Revision: 1840901 > > URL: http://svn.apache.org/viewvc?rev=1840901=rev > Log: > Review thread-safety > Document locking strategy > Fix a few issues: > - Iterators could throw ConcurrentModificationException > - Syncs on open/save didn't lock roles Map > Update with a view to supporting runtime reloading (BZ 58590) > This user db stuff was introduced in 4.1 and was never really updated since, nor were other implementations done. It survived because of the manager/jmx management it had. Do you think it would be useful to take it somewhere now, or is this only for 58590 ? Rémy
