tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Felix Schumacher <felix.schumac...@internetallee.de>
Subject Re: svn commit: r1703177 - in /tomcat/tc8.0.x/trunk: ./ java/org/apache/catalina/tribes/membership/Membership.java webapps/docs/changelog.xml
Date Tue, 15 Sep 2015 17:28:38 GMT
Hi Mark,

Am 15.09.2015 um 14:45 schrieb markt@apache.org:
> Author: markt
> Date: Tue Sep 15 12:45:48 2015
> New Revision: 1703177
>
> URL: http://svn.apache.org/r1703177
> Log:
> Use single object (membersLock) for all locking
> Make members volatile so single reads are safe
I am not sure, if it is enough to make members volatile. I think the 
read/write guarantees are only valid if you change the variable 
(reference) itself, not if you modify the contents of the array.

This might be a problem, if Arrays.sort sorts the array in place.

Another (old) problem might be that getMembers returns the intern array 
reference, which could be changed by the client and thus messing with 
the inner state of the class. And getMember(Member) could probably throw 
an IndexOutOfBoundException, when members would be decreased while the 
method iterates over it (probably unlikely).

Regards,
  Felix
> Reduce scope of locks where possible
> Expand scope of locks where necessary
>
> Modified:
>      tomcat/tc8.0.x/trunk/   (props changed)
>      tomcat/tc8.0.x/trunk/java/org/apache/catalina/tribes/membership/Membership.java
>      tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml
>
> Propchange: tomcat/tc8.0.x/trunk/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Tue Sep 15 12:45:48 2015
> @@ -1 +1 @@
> -/tomcat/trunk:1636524,1637156,1637176,1637188,1637331,1637684,1637695,1638720-1638725,1639653,1640010,1640083-1640084,1640088,1640275,1640322,1640347,1640361,1640365,1640403,1640410,1640652,1640655-1640658,1640688,1640700-1640883,1640903,1640976,1640978,1641000,1641026,1641038-1641039,1641051-1641052,1641058,1641064,1641300,1641369,1641374,1641380,1641486,1641634,1641656-1641692,1641704,1641707-1641718,1641720-1641722,1641735,1641981,1642233,1642280,1642554,1642564,1642595,1642606,1642668,1642679,1642697,1642699,1642766,1643002,1643045,1643054-1643055,1643066,1643121,1643128,1643206,1643209-1643210,1643216,1643249,1643270,1643283,1643309-1643310,1643323,1643365-1643366,1643370-1643371,1643465,1643474,1643536,1643570,1643634,1643649,1643651,1643654,1643675,1643731,1643733-1643734,1643761,1643766,1643814,1643937,1643963,1644017,1644169,1644201-1644203,1644321,1644323,1644516,1644523,1644529,1644535,1644730,1644768,1644784-1644785,1644790,1644793,1644815,1644884,1644886,1644890,1644892
>   ,1644910,1644924,1644929-1644930,1644935,1644989,1645011,1645247,1645355,1645357-1645358,1645455,1645465,1645469,1645471,1645473,1645475,1645486-1645488,1645626,1645641,1645685,1645743,1645763,1645951-1645953,1645955,1645993,1646098-1646106,1646178,1646220,1646302,1646304,1646420,1646470-1646471,1646476,1646559,1646717-1646723,1646773,1647026,1647042,1647530,1647655,1648304,1648815,1648907,1650081,1650365,1651116,1651120,1651280,1651470,1652938,1652970,1653041,1653471,1653550,1653574,1653797,1653815-1653816,1653819,1653840,1653857,1653888,1653972,1654013,1654030,1654050,1654123,1654148,1654159,1654513,1654515,1654517,1654522,1654524,1654725,1654735,1654766,1654785,1654851-1654852,1654978,1655122-1655124,1655126-1655127,1655129-1655130,1655132-1655133,1655312,1655438,1655441,1655454,1655558,1656087,1656299,1656319,1656331,1656345,1656350,1656590,1656648-1656650,1656657,1657041,1657054,1657374,1657492,1657510,1657565,1657580,1657584,1657586,1657589,1657592,1657607,1657609,1657682,1657
>   907,1658207,1658734,1658781,1658790,1658799,1658802,1658804,1658833,1658840,1658966,1659043,1659053,1659059,1659188-1659189,1659216,1659263,1659293,1659304,1659306-1659307,1659382,1659384,1659428,1659471,1659486,1659505,1659516,1659521,1659524,1659559,1659562,1659803,1659806,1659814,1659833,1659862,1659905,1659919,1659948,1659967,1659983-1659984,1660060,1660074,1660077,1660133,1660168,1660331-1660332,1660353,1660358,1660924,1661386,1661867,1661972,1661990,1662200,1662308-1662309,1662548,1662614,1662736,1662985,1662988-1662989,1663264,1663277,1663298,1663534,1663562,1663676,1663715,1663754,1663768,1663772,1663781,1663893,1663995,1664143,1664163,1664174,1664301,1664317,1664347,1664657,1664659,1664710,1664863-1664864,1664866,1665085,1665292,1665559,1665653,1665661,1665672,1665694,1665697,1665736,1665779,1665976-1665977,1665980-1665981,1665985-1665986,1665989,1665998,1666004,1666008,1666013,1666017,1666024,1666116,1666386-1666387,1666494,1666496,1666552,1666569,1666579,1666637,1666649,1
>   666757,1666966,1666972,1666985,1666995,1666997,1667292,1667402,1667406,1667546,1667615,1667630,1667636,1667688,1667764,1667871,1668026,1668135,1668193,1668593,1668596,1668630,1668639,1668843,1669353,1669370,1669451,1669800,1669838,1669876,1669882,1670394,1670433,1670591,1670598-1670600,1670610,1670631,1670719,1670724,1670726,1670730,1670940,1671112,1672272,1672284,1673754,1674294,1675461,1675486,1675594,1675830,1676231,1676250-1676251,1676364,1676381,1676393,1676479,1676525,1676552,1676615,1676630,1676634,1676721,1676926,1676943,1677140,1677802,1678011,1678162,1678174,1678339,1678426-1678427,1678694,1678701,1679534,1679708,1679710,1679716,1680034,1680246,1681056,1681123,1681138,1681280,1681283,1681286,1681450,1681697,1681701,1681729,1681770,1681779,1681793,1681807,1681837-1681838,1681854,1681862,1681958,1682028,1682033,1682311,1682315,1682317,1682320,1682324,1682330,1682842,1684172,1684366,1684383,1684526-1684527,1684549-1684550,1685556,1685591,1685739,1685744,1685772,1685816,168582
>   6,1685891,1687242,1687261,1687268,1687340,1688563,1688841,1688878,1688885,1688896,1688901,1689345-1689346,1689357,1689656,1689675-1689677,1689679,1689687,1689825,1689856,1689918,1690011,1690021,1690054,1690080,1690209,1691134,1691487,1691813,1692744-1692747,1692849,1693088,1693105,1693429,1693461,1694058,1694111,1694290,1694501,1694548,1694658,1694660,1694788,1694872,1694878,1695006,1695354,1695371,1695459,1695582,1695706,1695778,1696199,1696272,1696280,1696366-1696368,1696378,1696390,1696392,1696467,1700607,1700870,1700896,1700977,1701093,1701123,1701213,1701607,1701666,1701673,1701760-1701761,1701765,1701940,1702246,1702250,1702268,1702313,1702531,1702630-1702635,1702637-1702638,1702640,1702647,1702660,1702662,1702665-1702666,1702668,1702671-1702673,1702675-1702676,1702680,1702722,1702778,1702795,1702862,1702881,1702886,1702910,1702923,1702971,1702984,1703024,1703040,1703044,1703049-1703050,1703143,1703146,1703151,1703160
> +/tomcat/trunk:1636524,1637156,1637176,1637188,1637331,1637684,1637695,1638720-1638725,1639653,1640010,1640083-1640084,1640088,1640275,1640322,1640347,1640361,1640365,1640403,1640410,1640652,1640655-1640658,1640688,1640700-1640883,1640903,1640976,1640978,1641000,1641026,1641038-1641039,1641051-1641052,1641058,1641064,1641300,1641369,1641374,1641380,1641486,1641634,1641656-1641692,1641704,1641707-1641718,1641720-1641722,1641735,1641981,1642233,1642280,1642554,1642564,1642595,1642606,1642668,1642679,1642697,1642699,1642766,1643002,1643045,1643054-1643055,1643066,1643121,1643128,1643206,1643209-1643210,1643216,1643249,1643270,1643283,1643309-1643310,1643323,1643365-1643366,1643370-1643371,1643465,1643474,1643536,1643570,1643634,1643649,1643651,1643654,1643675,1643731,1643733-1643734,1643761,1643766,1643814,1643937,1643963,1644017,1644169,1644201-1644203,1644321,1644323,1644516,1644523,1644529,1644535,1644730,1644768,1644784-1644785,1644790,1644793,1644815,1644884,1644886,1644890,1644892
>   ,1644910,1644924,1644929-1644930,1644935,1644989,1645011,1645247,1645355,1645357-1645358,1645455,1645465,1645469,1645471,1645473,1645475,1645486-1645488,1645626,1645641,1645685,1645743,1645763,1645951-1645953,1645955,1645993,1646098-1646106,1646178,1646220,1646302,1646304,1646420,1646470-1646471,1646476,1646559,1646717-1646723,1646773,1647026,1647042,1647530,1647655,1648304,1648815,1648907,1650081,1650365,1651116,1651120,1651280,1651470,1652938,1652970,1653041,1653471,1653550,1653574,1653797,1653815-1653816,1653819,1653840,1653857,1653888,1653972,1654013,1654030,1654050,1654123,1654148,1654159,1654513,1654515,1654517,1654522,1654524,1654725,1654735,1654766,1654785,1654851-1654852,1654978,1655122-1655124,1655126-1655127,1655129-1655130,1655132-1655133,1655312,1655438,1655441,1655454,1655558,1656087,1656299,1656319,1656331,1656345,1656350,1656590,1656648-1656650,1656657,1657041,1657054,1657374,1657492,1657510,1657565,1657580,1657584,1657586,1657589,1657592,1657607,1657609,1657682,1657
>   907,1658207,1658734,1658781,1658790,1658799,1658802,1658804,1658833,1658840,1658966,1659043,1659053,1659059,1659188-1659189,1659216,1659263,1659293,1659304,1659306-1659307,1659382,1659384,1659428,1659471,1659486,1659505,1659516,1659521,1659524,1659559,1659562,1659803,1659806,1659814,1659833,1659862,1659905,1659919,1659948,1659967,1659983-1659984,1660060,1660074,1660077,1660133,1660168,1660331-1660332,1660353,1660358,1660924,1661386,1661867,1661972,1661990,1662200,1662308-1662309,1662548,1662614,1662736,1662985,1662988-1662989,1663264,1663277,1663298,1663534,1663562,1663676,1663715,1663754,1663768,1663772,1663781,1663893,1663995,1664143,1664163,1664174,1664301,1664317,1664347,1664657,1664659,1664710,1664863-1664864,1664866,1665085,1665292,1665559,1665653,1665661,1665672,1665694,1665697,1665736,1665779,1665976-1665977,1665980-1665981,1665985-1665986,1665989,1665998,1666004,1666008,1666013,1666017,1666024,1666116,1666386-1666387,1666494,1666496,1666552,1666569,1666579,1666637,1666649,1
>   666757,1666966,1666972,1666985,1666995,1666997,1667292,1667402,1667406,1667546,1667615,1667630,1667636,1667688,1667764,1667871,1668026,1668135,1668193,1668593,1668596,1668630,1668639,1668843,1669353,1669370,1669451,1669800,1669838,1669876,1669882,1670394,1670433,1670591,1670598-1670600,1670610,1670631,1670719,1670724,1670726,1670730,1670940,1671112,1672272,1672284,1673754,1674294,1675461,1675486,1675594,1675830,1676231,1676250-1676251,1676364,1676381,1676393,1676479,1676525,1676552,1676615,1676630,1676634,1676721,1676926,1676943,1677140,1677802,1678011,1678162,1678174,1678339,1678426-1678427,1678694,1678701,1679534,1679708,1679710,1679716,1680034,1680246,1681056,1681123,1681138,1681280,1681283,1681286,1681450,1681697,1681701,1681729,1681770,1681779,1681793,1681807,1681837-1681838,1681854,1681862,1681958,1682028,1682033,1682311,1682315,1682317,1682320,1682324,1682330,1682842,1684172,1684366,1684383,1684526-1684527,1684549-1684550,1685556,1685591,1685739,1685744,1685772,1685816,168582
>   6,1685891,1687242,1687261,1687268,1687340,1688563,1688841,1688878,1688885,1688896,1688901,1689345-1689346,1689357,1689656,1689675-1689677,1689679,1689687,1689825,1689856,1689918,1690011,1690021,1690054,1690080,1690209,1691134,1691487,1691813,1692744-1692747,1692849,1693088,1693105,1693429,1693461,1694058,1694111,1694290,1694501,1694548,1694658,1694660,1694788,1694872,1694878,1695006,1695354,1695371,1695459,1695582,1695706,1695778,1696199,1696272,1696280,1696366-1696368,1696378,1696390,1696392,1696467,1700607,1700870,1700896,1700977,1701093,1701123,1701213,1701607,1701666,1701673,1701760-1701761,1701765,1701940,1702246,1702250,1702268,1702313,1702531,1702630-1702635,1702637-1702638,1702640,1702647,1702660,1702662,1702665-1702666,1702668,1702671-1702673,1702675-1702676,1702680,1702722,1702778,1702795,1702862,1702881,1702886,1702910,1702923,1702971,1702984,1703024,1703040,1703044,1703049-1703050,1703143,1703146,1703151,1703160,1703164,1703167,1703174
>
> Modified: tomcat/tc8.0.x/trunk/java/org/apache/catalina/tribes/membership/Membership.java
> URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/catalina/tribes/membership/Membership.java?rev=1703177&r1=1703176&r2=1703177&view=diff
> ==============================================================================
> --- tomcat/tc8.0.x/trunk/java/org/apache/catalina/tribes/membership/Membership.java (original)
> +++ tomcat/tc8.0.x/trunk/java/org/apache/catalina/tribes/membership/Membership.java Tue
Sep 15 12:45:48 2015
> @@ -48,12 +48,12 @@ public class Membership implements Clone
>       /**
>        * A map of all the members in the cluster.
>        */
> -    protected HashMap<Member, MbrEntry> map = new HashMap<>();
> +    protected HashMap<Member, MbrEntry> map = new HashMap<>(); // Guarded
by membersLock
>   
>       /**
>        * A list of all the members in the cluster.
>        */
> -    protected Member[] members = EMPTY_MEMBERS;
> +    protected volatile Member[] members = EMPTY_MEMBERS; // Guarded by membersLock
>   
>       /**
>        * Comparator for sorting members by alive time.
> @@ -92,19 +92,21 @@ public class Membership implements Clone
>   
>       public Membership(Member local, Comparator<Member> comp, boolean includeLocal)
{
>           this.local = local;
> +        this.memberComparator = comp;
>           if (includeLocal) {
>               addMember(local);
>           }
> -        this.memberComparator = comp;
>       }
>   
>       /**
>        * Reset the membership and start over fresh. i.e., delete all the members
>        * and wait for them to ping again and join this membership.
>        */
> -    public synchronized void reset() {
> -        map.clear();
> -        members = EMPTY_MEMBERS ;
> +    public void reset() {
> +        synchronized (membersLock) {
> +            map.clear();
> +            members = EMPTY_MEMBERS ;
> +        }
>       }
>   
>       /**
> @@ -114,29 +116,31 @@ public class Membership implements Clone
>        * @return - true if this member is new to the cluster, false otherwise.<br>
>        * - false if this member is the local member or updated.
>        */
> -    public synchronized boolean memberAlive(Member member) {
> -        //ignore ourselves
> -        if ( member.equals(local)) {
> +    public boolean memberAlive(Member member) {
> +        // Ignore ourselves
> +        if (member.equals(local)) {
>               return false;
>           }
>   
>           boolean result = false;
> -        MbrEntry entry = map.get(member);
> -        if (entry == null) {
> -            entry = addMember(member);
> -            result = true;
> -       } else {
> -            //update the member alive time
> -            Member updateMember = entry.getMember();
> -            if(updateMember.getMemberAliveTime() != member.getMemberAliveTime()) {
> -                //update fields that can change
> -                updateMember.setMemberAliveTime(member.getMemberAliveTime());
> -                updateMember.setPayload(member.getPayload());
> -                updateMember.setCommand(member.getCommand());
> -                Arrays.sort(members, memberComparator);
> +        synchronized (membersLock) {
> +            MbrEntry entry = map.get(member);
> +            if (entry == null) {
> +                entry = addMember(member);
> +                result = true;
> +            } else {
> +                // Update the member alive time
> +                Member updateMember = entry.getMember();
> +                if (updateMember.getMemberAliveTime() != member.getMemberAliveTime())
{
> +                    // Update fields that can change
> +                    updateMember.setMemberAliveTime(member.getMemberAliveTime());
> +                    updateMember.setPayload(member.getPayload());
> +                    updateMember.setCommand(member.getCommand());
> +                    Arrays.sort(members, memberComparator);
> +                }
>               }
> +            entry.accessed();
>           }
> -        entry.accessed();
>           return result;
>       }
>   
> @@ -147,9 +151,9 @@ public class Membership implements Clone
>        *
>        * @return The member entry created for this new member.
>        */
> -    public synchronized MbrEntry addMember(Member member) {
> +    public MbrEntry addMember(Member member) {
> +        MbrEntry entry = new MbrEntry(member);
>           synchronized (membersLock) {
> -            MbrEntry entry = new MbrEntry(member);
>               if (!map.containsKey(member) ) {
>                   map.put(member, entry);
>                   Member results[] = new Member[members.length + 1];
> @@ -160,8 +164,8 @@ public class Membership implements Clone
>                   members = results;
>                   Arrays.sort(members, memberComparator);
>               }
> -            return entry;
>           }
> +        return entry;
>       }
>   
>       /**
> @@ -170,8 +174,8 @@ public class Membership implements Clone
>        * @param member The member to remove
>        */
>       public void removeMember(Member member) {
> -        map.remove(member);
>           synchronized (membersLock) {
> +            map.remove(member);
>               int n = -1;
>               for (int i = 0; i < members.length; i++) {
>                   if (members[i] == member || members[i].equals(member)) {
> @@ -198,33 +202,35 @@ public class Membership implements Clone
>        * @param maxtime - the max time a member can remain unannounced before it is considered
dead.
>        * @return the list of expired members
>        */
> -    public synchronized Member[] expire(long maxtime) {
> -        if (!hasMembers()) {
> -           return EMPTY_MEMBERS;
> -        }
> +    public Member[] expire(long maxtime) {
> +        synchronized (membersLock) {
> +            if (!hasMembers()) {
> +               return EMPTY_MEMBERS;
> +            }
>   
> -        ArrayList<Member> list = null;
> -        Iterator<MbrEntry> i = map.values().iterator();
> -        while (i.hasNext()) {
> -            MbrEntry entry = i.next();
> -            if (entry.hasExpired(maxtime)) {
> -                if (list == null) {
> -                    // Only need a list when members are expired (smaller gc)
> -                    list = new java.util.ArrayList<>();
> +            ArrayList<Member> list = null;
> +            Iterator<MbrEntry> i = map.values().iterator();
> +            while (i.hasNext()) {
> +                MbrEntry entry = i.next();
> +                if (entry.hasExpired(maxtime)) {
> +                    if (list == null) {
> +                        // Only need a list when members are expired (smaller gc)
> +                        list = new java.util.ArrayList<>();
> +                    }
> +                    list.add(entry.getMember());
>                   }
> -                list.add(entry.getMember());
>               }
> -        }
>   
> -        if (list != null) {
> -            Member[] result = new Member[list.size()];
> -            list.toArray(result);
> -            for (int j=0; j<result.length; j++) {
> -                removeMember(result[j]);
> +            if (list != null) {
> +                Member[] result = new Member[list.size()];
> +                list.toArray(result);
> +                for (int j=0; j<result.length; j++) {
> +                    removeMember(result[j]);
> +                }
> +                return result;
> +            } else {
> +                return EMPTY_MEMBERS ;
>               }
> -            return result;
> -        } else {
> -            return EMPTY_MEMBERS ;
>           }
>       }
>   
> @@ -235,14 +241,15 @@ public class Membership implements Clone
>        *         <code>false</code>
>        */
>       public boolean hasMembers() {
> -        return members.length > 0 ;
> +        return members.length > 0;
>       }
>   
>   
>       public Member getMember(Member mbr) {
> -        if (hasMembers()) {
> +        Member[] members = this.members;
> +        if (members.length > 0) {
>               Member result = null;
> -            for (int i = 0; i < this.members.length && result == null; i++)
{
> +            for (int i = 0; i < members.length && result == null; i++) {
>                   if (members[i].equals(mbr)) {
>                       result = members[i];
>                   }
> @@ -264,11 +271,7 @@ public class Membership implements Clone
>        * @return An array of the current members
>        */
>       public Member[] getMembers() {
> -        if (hasMembers()) {
> -            return members;
> -        } else {
> -            return EMPTY_MEMBERS;
> -        }
> +        return members;
>       }
>   
>       /**
>
> Modified: tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml?rev=1703177&r1=1703176&r2=1703177&view=diff
> ==============================================================================
> --- tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml Tue Sep 15 12:45:48 2015
> @@ -216,6 +216,10 @@
>           <bug>58381</bug>: Fix a rare data race in the <code>NioReceiver</code>.
>           (markt)
>         </fix>
> +      <fix>
> +        <bug>58382</bug>: Fix multiple rare data races in the default membership
> +        implementation. (markt)
> +      </fix>
>       </changelog>
>     </subsection>
>     <subsection name="jdbc-pool">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message