Bug 58590 - org.apache.catalina.realm.MemoryRealm can use backgroundProcess() to reload tomcat-users.xml when it changes
Summary: org.apache.catalina.realm.MemoryRealm can use backgroundProcess() to reload t...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: unspecified
Hardware: All All
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-05 17:30 UTC by Aidan
Modified: 2018-11-29 07:19 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aidan 2015-11-05 17:30:13 UTC
Based on a derived class that I created and tested successfully with Tomcat 8, I suggest the following lines be added to MemoryRealm in order to give it this capability:


    private Date _lastUpdate = null;
    private File _usersFile = null;


    public MemoryRealm()
    {
        _lastUpdate = new Date();
        _usersFile = new File( getPathname() );
    }


    /**
     * The default 10 second value for the Engine's backgroundProcessorDelay XML attribute
     * will cause this method to be called that often.
     */
    @Override
    public void backgroundProcess()
    {
        try {
            // only reload if file has changed since we last loaded it
            if( _lastUpdate.getTime() < _usersFile.lastModified() ) {
                _lastUpdate = new Date();

                log.info( "reloading " + getPathname() );

                stop();
                principals.clear();
                start(); // trigger a repopulation (from tomcat-users.xml)
            }
        }
        catch( Exception ex ) {
            log.error( "Failed to re-initialise realm: ", ex );
        }
    }
Comment 1 Konstantin Kolinko 2015-11-05 22:55:29 UTC
1. Code conventions:
http://tomcat.apache.org/getinvolved.html

Actually Sun's conventions but with spaces instead of tabs.

2. backgroundProcess() runs frequently. I see no need to perform this work on each run.

3. It shall work without stopping and starting the realm. Users should not be locked from the system while it reloads.

4. MemoryRealm is rarely used.  The usual configuration uses UserDatabaseRealm + MemoryUserDatabase (created by MemoryUserDatabaseFactory).

5. It should be possible to turn this feature on or off.

I do not know what the default should be.
On one hand we already have <Host autoDeploy="true"> so we are already checking hard drive by default.

On other hand, on production systems such setting is likely to be off by default (as there is an expectation that nobody will ever update that file).

6. There shall be an explicit method to reload and an explicit method to perform an up-to-date check - so that it were possible to call them via JMX.
Comment 2 Aidan 2015-11-05 23:34:51 UTC
(In reply to Konstantin Kolinko from comment #1)

Hi Konstantin,

> 1. Code conventions:
> http://tomcat.apache.org/getinvolved.html
> 
> Actually Sun's conventions but with spaces instead of tabs.

Ok, I can edit to conform with that.

> 2. backgroundProcess() runs frequently. I see no need to perform this work
> on each run.

Every 10 seconds for this container, by default, I believe - that doesn't seem too often to merely check the modified date of a file.

> 3. It shall work without stopping and starting the realm. Users should not
> be locked from the system while it reloads.

OK, fair point, I can try to address that.

> 4. MemoryRealm is rarely used.  The usual configuration uses
> UserDatabaseRealm + MemoryUserDatabase (created by
> MemoryUserDatabaseFactory).

True, but it wasn't clear to me how to effect the same change in UserDatabaseRealm and I was a bit pushed for time.

> 5. It should be possible to turn this feature on or off.

As I wasn't proposing this change for the default Realm I didn't think that was necessary.

> I do not know what the default should be.
> On one hand we already have <Host autoDeploy="true"> so we are already
> checking hard drive by default.
> 
> On other hand, on production systems such setting is likely to be off by
> default (as there is an expectation that nobody will ever update that file).

I think that setting only relates to .war files ?? AFAIK there is currently no mechanism to force re-reading of tomcat-users.xml without a restart. But I accept that if a user requires runtime user/role loading then they should probably use a JNDI or JDBC-based realm instead. However, I think it's nice to have an out-of-the-box alternative that has this capability. 
I wrote this patch at the bank where I work where we use Tomcat (with the APR) to serve several hundred internal users 24x5. We rarely update tomcat-users.xml but when we do we have to perform a disruptive restart. The patch fixes that, at least.
 
> 6. There shall be an explicit method to reload and an explicit method to
> perform an up-to-date check - so that it were possible to call them via JMX.

I hadn't thought of that. But as cool and useful as JMX is, it's beyond a lot of the casual users whom this patch is aimed at, I suspect.

Having said all that, I'm happy to rework it as best I can in order to address all your points or for you to simply reject it.

Kind Regards,
Aidan
Comment 3 Christopher Schultz 2015-11-06 11:59:07 UTC
Two things:

1. If possible, look at the UserDatabase class to see if you can get that to reload instead of the Realm. If you think about e.g. DatsSourceRealm, it makes more sense to "reload" the data source and not the realm itself.

2. Instead of calling _lastUpdate.getTime() each time, why not just store the native long value returned by it? (Also, you can then simply store the value of _usersFile.lastModified instead of creating a new Date object).

As for Konstantin being picky about your implementation... we get to be as picky about patch submissions as the submitters are willing to tolerate. Since you are motivated to get your patch accepted, we just want to make sure it's as high-quality as possible.

Thanks for your contributions!
Comment 4 Mark Thomas 2018-09-14 12:38:08 UTC
Implemented in trunk for 9.0.13 onwards.

Not back-ported as it would require changing the UserDatabase API as Java 7 doesn't support default method implementations.
Comment 5 recrute 2018-11-29 01:19:09 UTC
I just switch tomcat version from apache-tomcat-9.0.11 to apache-tomcat-9.0.13,
and then I found the tomcat instance could not work well again.

The error log shown:
org.apache.tomcat.jni.Error: 24: Too many open files

And here is the data from lsof:

[root@localhost versions]# lsof|grep "java"|wc -l
194186
[root@localhost versions]# lsof|grep "java"|grep "conf/tomcat-users.xml"|wc -l
152991

We can see there are too many handles from tomcat about "tomcat-users.xml",
so is there any wrong about apache-tomcat-9.0.13 on this case?
Comment 6 Mark Thomas 2018-11-29 07:19:08 UTC
See bug 62924.