On 15/09/2015 20:15, Mark Thomas wrote: > On 15/09/2015 18:28, Felix Schumacher wrote: >> Hi Mark, >> >> Am 15.09.2015 um 14:45 schrieb ma...@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. > > Correct. I took the comments in the previous code at face value and > assumed that Arrays.sort() would be applied before setting members. A > quick look shows that isn't the case. I'll do a quick review of the code > for similar issues and get them fixed. > >> 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). > > I think we can live with that. Fixing it would require defensive copies > or changing the API to use, for example, a read-only list. In think > either of those options would be too expensive.
Hmm. Looking through the code I can't actually see why we even need the ordering. The only places where getMemberAliveTime() is called is in the comparator and in memberAlive() when the updates take place. Given that, I think we just remove the ordering and the associated comparator. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
