Derby
  1. Derby
  2. DERBY-5582

Access denied (java.lang.RuntimePermission modifyThreadGroup) in IndexStatisticsDaemonImpl.schedule()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.8.3.0
    • Fix Version/s: 10.8.3.0
    • Component/s: Services
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      High Value Fix, Patch Available
    • Bug behavior facts:
      Regression

      Description

      I user reported this exception with 10.8.2.3 - (1212722) when running regression tests against 10.8.
      As soon as the Index Statistics Thread was initialized they got the stack trace below.

      There was some discussion of this issue on the dev list:
      http://old.nabble.com/Report-of-security-manager-issue-with-10.8-and-ndexStatisticsDaemonImpl.schedule-to33137398.html

      I assume the failure is in
      runningThread = new Thread(this, "index-stat-thread");

      Stack Trace:

      java.security.AccessControlException: Access denied
      (java.lang.RuntimePermission modifyThreadGroup)
      at
      java.security.AccessController.checkPermission(AccessController.java:108)
      at
      java.lang.SecurityManager.checkPermission(SecurityManager.java:544)
      at
      com.ibm.ws.security.core.SecurityManager.checkPermission(SecurityManager.java:208)
      at
      com.ibm.ws.security.core.SecurityManager.checkAccess(SecurityManager.java:407)
      at java.lang.ThreadGroup.checkAccess(ThreadGroup.java:226)
      at java.lang.Thread.initialize(Thread.java:345)
      at java.lang.Thread.<init>(Thread.java:281)
      at java.lang.Thread.<init>(Thread.java:179)
      at
      org.apache.derby.impl.services.daemon.IndexStatisticsDaemonImpl.schedule(Unknown
      Source)
      at
      org.apache.derby.impl.sql.GenericStatement.prepMinion(Unknown Source)
      at org.apache.derby.impl.sql.GenericStatement.prepare(Unknown
      Source)
      at
      org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(Unknown
      Source)
      at
      org.apache.derby.impl.jdbc.EmbedPreparedStatement.<init>(Unknown Source)
      at
      org.apache.derby.impl.jdbc.EmbedPreparedStatement20.<init>(Unknown Source)
      at
      org.apache.derby.impl.jdbc.EmbedPreparedStatement30.<init>(Unknown Source)
      at
      org.apache.derby.impl.jdbc.EmbedPreparedStatement40.<init>(Unknown Source)
      at
      org.apache.derby.jdbc.Driver40.newEmbedPreparedStatement(Unknown Source)
      at
      org.apache.derby.impl.jdbc.EmbedConnection.prepareStatement(Unknown Source)
      at
      org.apache.derby.impl.jdbc.EmbedConnection.prepareStatement(Unknown Source)
      at

      1. derby-5582_10_8_try1_diff.txt
        2 kB
        Kathey Marsden
      2. derby-5582_trunk_withtest_diff.txt
        12 kB
        Kathey Marsden
      3. derby-5582_trunk_withtest_diff.txt
        7 kB
        Kathey Marsden
      4. derby-5582_whitespace_diff.txt
        21 kB
        Kathey Marsden
      5. derby5582.policy
        0.1 kB
        Kathey Marsden
      6. Derby5582Runner.java
        3 kB
        Kathey Marsden
      7. MySecurityManager.java
        0.6 kB
        Kathey Marsden

        Activity

        Hide
        Kathey Marsden added a comment -

        I was incorrect in my early assessment that public Thread(Runnable target,
        String name)
        should not throw a Security exception.

        I got this comment from the user which I think is a fair assessment. Derby should use the derby group for our thread. I gave them a test build with the change Kristian recommended and will see if that resolves the issue. I am having a bit of trouble getting a stand alone test case, but will keep working on that.

        According to the javadocs for Thread, that method effectively calls the following signature with TG=null
        public Thread(ThreadGroup group,
        Runnable target,
        String name)
        For which the javadocs say:
        "If group is null and there is a security manager, the group is determined by the security manager's getThreadGroup method. If group is null and there is not a security manager, or the security manager's getThreadGroup method returns null, the group is set to be the same ThreadGroup as the thread that is creating the new thread.
        If there is a security manager, its checkAccess method is called with the ThreadGroup as its argument."
        Based on the javadocs for the signature called, group is null, thus the group is determined by the security manager's getThreadGroup method. Since our sec mgr doesn't override the base sec mgr's getThreadGroup(), it's behavior is dependent on that impl. If it returned null, the group would then be that of the calling thread,
        which based on the stack is a worker thread from the thread pool executing an ejb call. Since there is a sec mgr, checkAccess is called. Since the thread is our ejb thread, not a Derby thread, I'm guessing that IndexStatisticsDaemonImpl.schedule() call doesn't have perms to add a new Thread to the ThreadGroup? So maybe the call
        to new up the statistics thread needs to explicitly use the Derby TG?

        Show
        Kathey Marsden added a comment - I was incorrect in my early assessment that public Thread(Runnable target, String name) should not throw a Security exception. I got this comment from the user which I think is a fair assessment. Derby should use the derby group for our thread. I gave them a test build with the change Kristian recommended and will see if that resolves the issue. I am having a bit of trouble getting a stand alone test case, but will keep working on that. According to the javadocs for Thread, that method effectively calls the following signature with TG=null public Thread(ThreadGroup group, Runnable target, String name) For which the javadocs say: "If group is null and there is a security manager, the group is determined by the security manager's getThreadGroup method. If group is null and there is not a security manager, or the security manager's getThreadGroup method returns null, the group is set to be the same ThreadGroup as the thread that is creating the new thread. If there is a security manager, its checkAccess method is called with the ThreadGroup as its argument." Based on the javadocs for the signature called, group is null, thus the group is determined by the security manager's getThreadGroup method. Since our sec mgr doesn't override the base sec mgr's getThreadGroup(), it's behavior is dependent on that impl. If it returned null, the group would then be that of the calling thread, which based on the stack is a worker thread from the thread pool executing an ejb call. Since there is a sec mgr, checkAccess is called. Since the thread is our ejb thread, not a Derby thread, I'm guessing that IndexStatisticsDaemonImpl.schedule() call doesn't have perms to add a new Thread to the ThreadGroup? So maybe the call to new up the statistics thread needs to explicitly use the Derby TG?
        Hide
        Kathey Marsden added a comment -

        derby-5582_10_8_try1_diff.txt
        Attaching the 10.8 patch that I gave the user to try changing the new Thread call to
        Monitor.getMonitor().getDaemonThread(this, "index-stat-thread", false);

        Not ready for commit. I don't know if it works and need to add tests.

        Show
        Kathey Marsden added a comment - derby-5582_10_8_try1_diff.txt Attaching the 10.8 patch that I gave the user to try changing the new Thread call to Monitor.getMonitor().getDaemonThread(this, "index-stat-thread", false); Not ready for commit. I don't know if it works and need to add tests.
        Hide
        Kristian Waagan added a comment -

        KM> Since there is a sec mgr, checkAccess is called. Since the thread is our ejb thread, not a Derby thread, I'm guessing that IndexStatisticsDaemonImpl.schedule() call doesn't have perms to add a new Thread to the ThreadGroup?

        It's a bit hard to say which preconditions must be met before the exception is actually thrown. From SecurityManager.checkAccess(ThreadGroup g):
        """
        If the thread group argument is the system thread group ( has a null parent) then this method calls checkPermission with the RuntimePermission("modifyThreadGroup") permission. If the thread group argument is not the system thread group, this method just returns silently.
        """

        Note that the documentation separates only between the system thread group and other thread groups. In my mind the thread pool referred to above would have to be part of the system thread group for the exception to be raised, unless there are custom security measures in place.
        Which JVM was the issue seen on?

        KM> So maybe the call to new up the statistics thread needs to explicitly use the Derby TG?

        Yes, I think that's the right thing to do in any case. That could also make it easier for someone wanting to implement a stricter security manager (i.e to allow only some components in a stack to create new threads).

        Show
        Kristian Waagan added a comment - KM> Since there is a sec mgr, checkAccess is called. Since the thread is our ejb thread, not a Derby thread, I'm guessing that IndexStatisticsDaemonImpl.schedule() call doesn't have perms to add a new Thread to the ThreadGroup? It's a bit hard to say which preconditions must be met before the exception is actually thrown. From SecurityManager.checkAccess(ThreadGroup g): """ If the thread group argument is the system thread group ( has a null parent) then this method calls checkPermission with the RuntimePermission("modifyThreadGroup") permission. If the thread group argument is not the system thread group, this method just returns silently. """ Note that the documentation separates only between the system thread group and other thread groups. In my mind the thread pool referred to above would have to be part of the system thread group for the exception to be raised, unless there are custom security measures in place. Which JVM was the issue seen on? KM> So maybe the call to new up the statistics thread needs to explicitly use the Derby TG? Yes, I think that's the right thing to do in any case. That could also make it easier for someone wanting to implement a stricter security manager (i.e to allow only some components in a stack to create new threads).
        Hide
        Kathey Marsden added a comment -

        The patch corrected the problem in the user environment. I am going to go ahead and commit while working on a custom SecurityManager reproduction so I can get them the fix quickly.

        Show
        Kathey Marsden added a comment - The patch corrected the problem in the user environment. I am going to go ahead and commit while working on a custom SecurityManager reproduction so I can get them the fix quickly.
        Hide
        Kathey Marsden added a comment -

        Here is a reproduction for the issue. MySecurityManager has a checkAccess(ThreadGroup tg) method which prevents creation of the index-stat thread with the default (parent) Thread Group.

        $ java Derby5582Runner
        currentGroup:main
        tg group: main
        currentGroup:main
        tg group: derby.daemons
        currentGroup:main
        tg group: derby.daemons
        currentGroup:system
        tg group: system
        currentGroup:main
        tg group: main
        currentGroup:main
        tg group: main
        currentGroup:main
        tg group: main
        currentGroup:main
        tg group: main
        initialized derby
        currentGroup:main
        tg group: main
        currentGroup:main
        tg group: privtg
        currentGroup:main
        tg group: privtg
        currentGroup:system
        Enter run()
        tg group: system
        currentGroup:privtg
        tg group: derby.daemons
        Created database
        Firing off the worker thread
        Starting index-stat thread
        currentGroup:privtg
        tg group: privtg
        throwing security exception
        got exception
        java.sql.SQLException: Java exception: 'No permission to private ThreadGroup privtg: java.lang.SecurityException'.
        at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98)
        at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
        at org.apache.derby.impl.jdbc.Util.javaException(Util.java:299)
        at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:436)
        at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:353)
        at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2288)
        at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:82)
        at org.apache.derby.impl.jdbc.EmbedPreparedStatement.<init>(EmbedPreparedStatement.java:153)
        at org.apache.derby.jdbc.Driver40.newEmbedPreparedStatement(Driver40.java:107)
        at org.apache.derby.impl.jdbc.EmbedConnection.prepareStatement(EmbedConnection.java:1613)
        at org.apache.derby.impl.jdbc.EmbedConnection.prepareStatement(EmbedConnection.java:1441)
        at Derby5582Runner.run(Derby5582Runner.java:43)
        at java.lang.Thread.run(Thread.java:736)
        Caused by: java.sql.SQLException: Java exception: 'No permission to private ThreadGroup privtg: java.lang.SecurityExcept
        ion'.
        at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:42)
        at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:12
        2)
        at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:71)
        ... 12 more
        Caused by: java.lang.SecurityException: No permission to private ThreadGroup privtg
        at MySecurityManager.checkAccess(MySecurityManager.java:10)
        at java.lang.ThreadGroup.checkAccess(ThreadGroup.java:226)
        at java.lang.Thread.initialize(Thread.java:331)
        at java.lang.Thread.<init>(Thread.java:267)
        at java.lang.Thread.<init>(Thread.java:165)
        at org.apache.derby.impl.services.daemon.IndexStatisticsDaemonImpl.schedule(IndexStatisticsDaemonImpl.java:257)
        at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:569)
        at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:97)
        at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConne
        ctionContext.java:1103)
        at org.apache.derby.impl.jdbc.EmbedPreparedStatement.<init>(EmbedPreparedStatement.java:134)
        ... 5 more
        run done

        Show
        Kathey Marsden added a comment - Here is a reproduction for the issue. MySecurityManager has a checkAccess(ThreadGroup tg) method which prevents creation of the index-stat thread with the default (parent) Thread Group. $ java Derby5582Runner currentGroup:main tg group: main currentGroup:main tg group: derby.daemons currentGroup:main tg group: derby.daemons currentGroup:system tg group: system currentGroup:main tg group: main currentGroup:main tg group: main currentGroup:main tg group: main currentGroup:main tg group: main initialized derby currentGroup:main tg group: main currentGroup:main tg group: privtg currentGroup:main tg group: privtg currentGroup:system Enter run() tg group: system currentGroup:privtg tg group: derby.daemons Created database Firing off the worker thread Starting index-stat thread currentGroup:privtg tg group: privtg throwing security exception got exception java.sql.SQLException: Java exception: 'No permission to private ThreadGroup privtg: java.lang.SecurityException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.javaException(Util.java:299) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:436) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:353) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2288) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:82) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.<init>(EmbedPreparedStatement.java:153) at org.apache.derby.jdbc.Driver40.newEmbedPreparedStatement(Driver40.java:107) at org.apache.derby.impl.jdbc.EmbedConnection.prepareStatement(EmbedConnection.java:1613) at org.apache.derby.impl.jdbc.EmbedConnection.prepareStatement(EmbedConnection.java:1441) at Derby5582Runner.run(Derby5582Runner.java:43) at java.lang.Thread.run(Thread.java:736) Caused by: java.sql.SQLException: Java exception: 'No permission to private ThreadGroup privtg: java.lang.SecurityExcept ion'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:42) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:12 2) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:71) ... 12 more Caused by: java.lang.SecurityException: No permission to private ThreadGroup privtg at MySecurityManager.checkAccess(MySecurityManager.java:10) at java.lang.ThreadGroup.checkAccess(ThreadGroup.java:226) at java.lang.Thread.initialize(Thread.java:331) at java.lang.Thread.<init>(Thread.java:267) at java.lang.Thread.<init>(Thread.java:165) at org.apache.derby.impl.services.daemon.IndexStatisticsDaemonImpl.schedule(IndexStatisticsDaemonImpl.java:257) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:569) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:97) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConne ctionContext.java:1103) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.<init>(EmbedPreparedStatement.java:134) ... 5 more run done
        Hide
        Kristian Waagan added a comment -

        Just for clarity, was the original exception throw due to a custom SecurityManager in the user environent, or because the VM chose (for whatever reason) the system thread group as the home for the new thread?

        Show
        Kristian Waagan added a comment - Just for clarity, was the original exception throw due to a custom SecurityManager in the user environent, or because the VM chose (for whatever reason) the system thread group as the home for the new thread?
        Hide
        Kathey Marsden added a comment -

        They do have a custom SecurityManager (From the trace :com.ibm.ws.security.core.SecurityManager)
        I don't have access to that code but was told that it would prevent access to the ejb thread group which would be the parent when Derby was run. My repro doesn't give the exact same error regarding modifyThreadGroup, but I think it is generally a representation of what was happening in their environment. Probably I am not throwing the right Exception, exactly to match their symptom.

        One thing I wondered about is why we create the index statistics thread in a lazy fashion. I wonder if it might be safer to just create it up front to make any issues apparent and avoid any context related issues when it kicks in.

        Show
        Kathey Marsden added a comment - They do have a custom SecurityManager (From the trace :com.ibm.ws.security.core.SecurityManager) I don't have access to that code but was told that it would prevent access to the ejb thread group which would be the parent when Derby was run. My repro doesn't give the exact same error regarding modifyThreadGroup, but I think it is generally a representation of what was happening in their environment. Probably I am not throwing the right Exception, exactly to match their symptom. One thing I wondered about is why we create the index statistics thread in a lazy fashion. I wonder if it might be safer to just create it up front to make any issues apparent and avoid any context related issues when it kicks in.
        Hide
        Kristian Waagan added a comment -

        Ok.
        Explicitly specifying the thread group to create the new thread should take care of most scenarios, like the one reported by this user.
        There's one other thing to consider. There's a group of operations / method calls that go through check-methods in the security manager, and most of the time these check-methods simply quietly return. Examples are (all relevant to Derby) new ThreadGroup(...), Thread.setDaemon(), and new Thread(...). It is rare (based on the number of error reports we've seen) but possible to write custom check-methods that would require permissions to be granted for these operations even if they don't involve system threads or system thread groups. By wrapping these calls in doPrivileged-blocks, we would make it easier to write such custom check-methods.
        Is this something that sounds useful?

        I can try to write a sample to make it easier to understand the issue. Note that this is different from, but also similar to, the issue reported by this JIRA. The custom security manager of the system used by the user was protecting one or more thread groups from modification, whereas what I'm talking about here is restricting sensitive thread operations in a broader sense (i.e. you would have to grant certain permissions to all code bases creating thread groups and/or threads).

        I think when to create the index statistics thread was discussed during the development of the feature. The conclusion was that we didn't want another per-database thread, mostly idle, hanging around.

        Show
        Kristian Waagan added a comment - Ok. Explicitly specifying the thread group to create the new thread should take care of most scenarios, like the one reported by this user. There's one other thing to consider. There's a group of operations / method calls that go through check-methods in the security manager, and most of the time these check-methods simply quietly return. Examples are (all relevant to Derby) new ThreadGroup(...), Thread.setDaemon(), and new Thread(...). It is rare (based on the number of error reports we've seen) but possible to write custom check-methods that would require permissions to be granted for these operations even if they don't involve system threads or system thread groups. By wrapping these calls in doPrivileged-blocks, we would make it easier to write such custom check-methods. Is this something that sounds useful? I can try to write a sample to make it easier to understand the issue. Note that this is different from, but also similar to, the issue reported by this JIRA. The custom security manager of the system used by the user was protecting one or more thread groups from modification, whereas what I'm talking about here is restricting sensitive thread operations in a broader sense (i.e. you would have to grant certain permissions to all code bases creating thread groups and/or threads). I think when to create the index statistics thread was discussed during the development of the feature. The conclusion was that we didn't want another per-database thread, mostly idle, hanging around.
        Hide
        Kathey Marsden added a comment - - edited

        Kristian asked.
        >By wrapping these calls in doPrivileged-blocks, we would make it easier to write such custom check-methods.
        Is this something that sounds useful?

        I think it would be good and correct to wrap them and could potentially be useful. DERBY-5571 touches on this but probably need a more general issue or change that one to cover the other calls.

        I think that might be a great newcomer task

        Show
        Kathey Marsden added a comment - - edited Kristian asked. >By wrapping these calls in doPrivileged-blocks, we would make it easier to write such custom check-methods. Is this something that sounds useful? I think it would be good and correct to wrap them and could potentially be useful. DERBY-5571 touches on this but probably need a more general issue or change that one to cover the other calls. I think that might be a great newcomer task
        Hide
        Kathey Marsden added a comment -

        Attached is the trunk diff with the test. derby-5582_trunk_withtest_diff.txt I am not sure my checkAccess method is quite right. I would prefer to get the actual modifyThreadGroup message to be thrown.

        Please review

        Show
        Kathey Marsden added a comment - Attached is the trunk diff with the test. derby-5582_trunk_withtest_diff.txt I am not sure my checkAccess method is quite right. I would prefer to get the actual modifyThreadGroup message to be thrown. Please review
        Hide
        Kristian Waagan added a comment -

        Kathey, I can't find the actual test code in the diff you attached. Can you provide a new diff?

        Thanks

        Show
        Kristian Waagan added a comment - Kathey, I can't find the actual test code in the diff you attached. Can you provide a new diff? Thanks
        Hide
        Kathey Marsden added a comment -

        My apologies. Here it is.

        Show
        Kathey Marsden added a comment - My apologies. Here it is.
        Hide
        Kathey Marsden added a comment -

        I went ahead and checked this in to trunk, but would appreciate review. I can submit a follow-up patch with any needed changes to the tests

        Show
        Kathey Marsden added a comment - I went ahead and checked this in to trunk, but would appreciate review. I can submit a follow-up patch with any needed changes to the tests
        Hide
        Kristian Waagan added a comment -

        There are some formatting issues in the patch (i.e. use of tabs, especially mixing spaces and tabs for indentation on the same line), and I was wondering why this condition is included in the checkAccess(ThreadGroup g) method:
        !currentGroup.getName().equals("main")
        From what I can understand, it's there to stop the thread/-group initialization in testDerby5582 from failing. The if then expresses "deny operation if target thread group is 'privtg' and the current thread is not in the threadgroup 'main'".

        Look good to me otherwise.

        Show
        Kristian Waagan added a comment - There are some formatting issues in the patch (i.e. use of tabs, especially mixing spaces and tabs for indentation on the same line), and I was wondering why this condition is included in the checkAccess(ThreadGroup g) method: !currentGroup.getName().equals("main") From what I can understand, it's there to stop the thread/-group initialization in testDerby5582 from failing. The if then expresses "deny operation if target thread group is 'privtg' and the current thread is not in the threadgroup 'main'". Look good to me otherwise.
        Hide
        Kathey Marsden added a comment -

        Thank you Kristian for looking at the patch. That is correct regarding the condition. We want the main thread to be able to make the original thread created in main to be in the privtg group but not other ones.

        Regarding the formatting. SecurityManagerSetup has become such a mix of tabs and spaces that it is hard to figure out. Would anyone object to a reformating patch to change this one to just spaces. You lose some history in annotation on trunk but could always go back to other branches for the annotation if that is needed.

        Show
        Kathey Marsden added a comment - Thank you Kristian for looking at the patch. That is correct regarding the condition. We want the main thread to be able to make the original thread created in main to be in the privtg group but not other ones. Regarding the formatting. SecurityManagerSetup has become such a mix of tabs and spaces that it is hard to figure out. Would anyone object to a reformating patch to change this one to just spaces. You lose some history in annotation on trunk but could always go back to other branches for the annotation if that is needed.
        Hide
        Kristian Waagan added a comment -

        I was thinking of the lines changed by the patch only, but I agree it's a bit more work now that it has been committed (i.e. no longer a simple search and replace).
        Looking at the SecurityManager-class, it has gone though quite a few modifications over time. Maybe it's better to leave things as they are and do the reformatting at a single point in the future ([1])?

        [1] May possibly be very distant. Maybe version 11, whenever that comes along, is the first opportunity?

        Show
        Kristian Waagan added a comment - I was thinking of the lines changed by the patch only, but I agree it's a bit more work now that it has been committed (i.e. no longer a simple search and replace). Looking at the SecurityManager-class, it has gone though quite a few modifications over time. Maybe it's better to leave things as they are and do the reformatting at a single point in the future ( [1] )? [1] May possibly be very distant. Maybe version 11, whenever that comes along, is the first opportunity?
        Hide
        Mike Matrigali added a comment -

        personally I am ok with changing an inconsistent file to consistent fot tab vs spaces, especially if you are actively working on the file and it is causing
        problems as above. I like the change to be a separate checkin from the bug fix to make it clear what you are doing. When I do it I try to
        add some extra value by fixing comments, maybe adding some javadoc where it is missing, ... Long term I think we agreed that spaces is
        the right direction.

        As you point out there is some loss of history in trunk, so not ready to mass do this for the sake of doing it. But again if you are working on a file
        and it is inconsistent, and it makes it easier for you to work on the file, i say go ahead.

        Show
        Mike Matrigali added a comment - personally I am ok with changing an inconsistent file to consistent fot tab vs spaces, especially if you are actively working on the file and it is causing problems as above. I like the change to be a separate checkin from the bug fix to make it clear what you are doing. When I do it I try to add some extra value by fixing comments, maybe adding some javadoc where it is missing, ... Long term I think we agreed that spaces is the right direction. As you point out there is some loss of history in trunk, so not ready to mass do this for the sake of doing it. But again if you are working on a file and it is inconsistent, and it makes it easier for you to work on the file, i say go ahead.
        Hide
        Kathey Marsden added a comment -

        I have to say eclipse is pretty infuriating in this regard. I try to switch it back and forth based on where I am but it insists on odd things. I will fix the tab in AutomaticIndexStatisticsTest and leave SecurityManagerSetup reformating for now. Looking at history of this file I see Kristian seems to take the approach of always using spaces for new lines, even when the surrounding code is tabs. Maybe that is the best way to go.

        Show
        Kathey Marsden added a comment - I have to say eclipse is pretty infuriating in this regard. I try to switch it back and forth based on where I am but it insists on odd things. I will fix the tab in AutomaticIndexStatisticsTest and leave SecurityManagerSetup reformating for now. Looking at history of this file I see Kristian seems to take the approach of always using spaces for new lines, even when the surrounding code is tabs. Maybe that is the best way to go.
        Hide
        Dag H. Wanvik added a comment -

        > I see Kristian seems to take the approach of always using spaces for new lines, even when the surrounding code is tabs

        Knut and I also switched to this some time ago.

        Show
        Dag H. Wanvik added a comment - > I see Kristian seems to take the approach of always using spaces for new lines, even when the surrounding code is tabs Knut and I also switched to this some time ago.
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update: close all resolved issues that haven't had any activity the last year]

        Show
        Knut Anders Hatlen added a comment - [bulk update: close all resolved issues that haven't had any activity the last year]

          People

          • Assignee:
            Kathey Marsden
            Reporter:
            Kathey Marsden
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development