On 15/09/2015 20:38, Mark Thomas wrote: > 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.
Scratch that. The an alternative comparator is passed in to the constructor in some cases that does appear to be used. Looking at the source I can't help but think there is scope to improve the NonBlockingCoordinator implementation but that is something for another day. I have the improvements to this class ready to commit. I'll get that done shortly. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
