You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Christopher Schultz <ch...@christopherschultz.net> on 2015/03/09 17:19:33 UTC
Impossible branch in
o.a.t.websocket.server.WsServerContainer.addEndpoint
All,
I was looking at https://bz.apache.org/bugzilla/show_bug.cgi?id=57676 (a
simple proposed patch to improve an error message) and I was trying to
figure out what to do with a bit of code in addEndpoint. Reading the
code, I think I've spotted a problem:
[187] UriTemplate uriTemplate = new UriTemplate(path);
if (uriTemplate.hasParameters()) {
Integer key = Integer.valueOf(uriTemplate.getSegmentCount());
SortedSet<TemplatePathMatch> templateMatches =
configTemplateMatchMap.get(key);
if (templateMatches == null) {
// Ensure that if concurrent threads execute this block they
// both end up using the same TreeSet instance
templateMatches = new TreeSet<>(
TemplatePathMatchComparator.getInstance());
configTemplateMatchMap.putIfAbsent(key, templateMatches);
templateMatches = configTemplateMatchMap.get(key);
}
[200] if (!templateMatches.add(new TemplatePathMatch(sec,
uriTemplate))) {
// Duplicate uriTemplate;
throw new DeploymentException(
sm.getString("serverContainer.duplicatePaths", path,
sec.getEndpointClass(),
sec.getEndpointClass()));
}
}
The problem is on line 200, where we check to see if
templateMatches.add() returns false (meaning that the object we added to
the set replaced one that was already there).
I don't believe this branch can /ever/ be reached because the
TemplatePathMatch class doesn't override Object.equals(), thus no two
objects of that type will ever equal each other. Therefore, a new
TemplatePathMatch object will always be added to the set.
I'm not sure what the implications are of multiple TemplatePathMatch
objects being in that set, but it looks like it's bad (we throw an
exception in that case), so someone with a better understanding of such
things might want to take a look.
(Also, if anyone would care to comment on how to get the "pre-existing"
match for the error message -- which is different than current-trunk as
it's what I'm working on -- I'd love some insight.)
Thanks,
-chris
Re: Impossible branch in
o.a.t.websocket.server.WsServerContainer.addEndpoint
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 3/17/15 4:25 PM, Mark Thomas wrote:
>>> On 3/9/15 12:19 PM, Christopher Schultz wrote:
>>>> All,
>>>>
>>>> I was looking at https://bz.apache.org/bugzilla/show_bug.cgi?id=57676 (a
>>>> simple proposed patch to improve an error message) and I was trying to
>>>> figure out what to do with a bit of code in addEndpoint. Reading the
>>>> code, I think I've spotted a problem:
>>>>
>>>> [187] UriTemplate uriTemplate = new UriTemplate(path);
>>>> if (uriTemplate.hasParameters()) {
>>>> Integer key = Integer.valueOf(uriTemplate.getSegmentCount());
>>>> SortedSet<TemplatePathMatch> templateMatches =
>>>> configTemplateMatchMap.get(key);
>>>> if (templateMatches == null) {
>>>> // Ensure that if concurrent threads execute this block they
>>>> // both end up using the same TreeSet instance
>>>> templateMatches = new TreeSet<>(
>>>> TemplatePathMatchComparator.getInstance());
>>>> configTemplateMatchMap.putIfAbsent(key, templateMatches);
>>>> templateMatches = configTemplateMatchMap.get(key);
>>>> }
>>>> [200] if (!templateMatches.add(new TemplatePathMatch(sec,
>>>> uriTemplate))) {
>>>> // Duplicate uriTemplate;
>>>> throw new DeploymentException(
>>>> sm.getString("serverContainer.duplicatePaths", path,
>>>> sec.getEndpointClass(),
>>>> sec.getEndpointClass()));
>>>> }
>>>> }
>>>>
>>>> The problem is on line 200, where we check to see if
>>>> templateMatches.add() returns false (meaning that the object we added to
>>>> the set replaced one that was already there).
>>>>
>>>> I don't believe this branch can /ever/ be reached because the
>>>> TemplatePathMatch class doesn't override Object.equals(), thus no two
>>>> objects of that type will ever equal each other. Therefore, a new
>>>> TemplatePathMatch object will always be added to the set.
>
> Your analysis is flawed.
>
> The only place values are added to configTemplateMatchMap is on line
> 197. The value object is always the TreeSet constructed on the previous
> line with a TemplatePathMatchComparator.
>
> That TemplatePathMatch doesn't override equals is therefore irrelevant.
> The Comparator will always be used.
+1
I wasn't considering the comparator being used by the collection itself.
Thanks,
-chris
Re: Impossible branch in o.a.t.websocket.server.WsServerContainer.addEndpoint
Posted by Mark Thomas <ma...@apache.org>.
>> On 3/9/15 12:19 PM, Christopher Schultz wrote:
>>> All,
>>>
>>> I was looking at https://bz.apache.org/bugzilla/show_bug.cgi?id=57676 (a
>>> simple proposed patch to improve an error message) and I was trying to
>>> figure out what to do with a bit of code in addEndpoint. Reading the
>>> code, I think I've spotted a problem:
>>>
>>> [187] UriTemplate uriTemplate = new UriTemplate(path);
>>> if (uriTemplate.hasParameters()) {
>>> Integer key = Integer.valueOf(uriTemplate.getSegmentCount());
>>> SortedSet<TemplatePathMatch> templateMatches =
>>> configTemplateMatchMap.get(key);
>>> if (templateMatches == null) {
>>> // Ensure that if concurrent threads execute this block they
>>> // both end up using the same TreeSet instance
>>> templateMatches = new TreeSet<>(
>>> TemplatePathMatchComparator.getInstance());
>>> configTemplateMatchMap.putIfAbsent(key, templateMatches);
>>> templateMatches = configTemplateMatchMap.get(key);
>>> }
>>> [200] if (!templateMatches.add(new TemplatePathMatch(sec,
>>> uriTemplate))) {
>>> // Duplicate uriTemplate;
>>> throw new DeploymentException(
>>> sm.getString("serverContainer.duplicatePaths", path,
>>> sec.getEndpointClass(),
>>> sec.getEndpointClass()));
>>> }
>>> }
>>>
>>> The problem is on line 200, where we check to see if
>>> templateMatches.add() returns false (meaning that the object we added to
>>> the set replaced one that was already there).
>>>
>>> I don't believe this branch can /ever/ be reached because the
>>> TemplatePathMatch class doesn't override Object.equals(), thus no two
>>> objects of that type will ever equal each other. Therefore, a new
>>> TemplatePathMatch object will always be added to the set.
Your analysis is flawed.
The only place values are added to configTemplateMatchMap is on line
197. The value object is always the TreeSet constructed on the previous
line with a TemplatePathMatchComparator.
That TemplatePathMatch doesn't override equals is therefore irrelevant.
The Comparator will always be used.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Impossible branch in o.a.t.websocket.server.WsServerContainer.addEndpoint
Posted by Mark Thomas <ma...@apache.org>.
On 17/03/2015 14:45, Christopher Schultz wrote:
> Ping
Sorry. This dropped off my radar while I was trying to figure out the
APR crash.
I'll take a look later today.
Mark
>
> On 3/9/15 12:19 PM, Christopher Schultz wrote:
>> All,
>>
>> I was looking at https://bz.apache.org/bugzilla/show_bug.cgi?id=57676 (a
>> simple proposed patch to improve an error message) and I was trying to
>> figure out what to do with a bit of code in addEndpoint. Reading the
>> code, I think I've spotted a problem:
>>
>> [187] UriTemplate uriTemplate = new UriTemplate(path);
>> if (uriTemplate.hasParameters()) {
>> Integer key = Integer.valueOf(uriTemplate.getSegmentCount());
>> SortedSet<TemplatePathMatch> templateMatches =
>> configTemplateMatchMap.get(key);
>> if (templateMatches == null) {
>> // Ensure that if concurrent threads execute this block they
>> // both end up using the same TreeSet instance
>> templateMatches = new TreeSet<>(
>> TemplatePathMatchComparator.getInstance());
>> configTemplateMatchMap.putIfAbsent(key, templateMatches);
>> templateMatches = configTemplateMatchMap.get(key);
>> }
>> [200] if (!templateMatches.add(new TemplatePathMatch(sec,
>> uriTemplate))) {
>> // Duplicate uriTemplate;
>> throw new DeploymentException(
>> sm.getString("serverContainer.duplicatePaths", path,
>> sec.getEndpointClass(),
>> sec.getEndpointClass()));
>> }
>> }
>>
>> The problem is on line 200, where we check to see if
>> templateMatches.add() returns false (meaning that the object we added to
>> the set replaced one that was already there).
>>
>> I don't believe this branch can /ever/ be reached because the
>> TemplatePathMatch class doesn't override Object.equals(), thus no two
>> objects of that type will ever equal each other. Therefore, a new
>> TemplatePathMatch object will always be added to the set.
>>
>> I'm not sure what the implications are of multiple TemplatePathMatch
>> objects being in that set, but it looks like it's bad (we throw an
>> exception in that case), so someone with a better understanding of such
>> things might want to take a look.
>>
>> (Also, if anyone would care to comment on how to get the "pre-existing"
>> match for the error message -- which is different than current-trunk as
>> it's what I'm working on -- I'd love some insight.)
>>
>> Thanks,
>> -chris
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Impossible branch in
o.a.t.websocket.server.WsServerContainer.addEndpoint
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Ping
On 3/9/15 12:19 PM, Christopher Schultz wrote:
> All,
>
> I was looking at https://bz.apache.org/bugzilla/show_bug.cgi?id=57676 (a
> simple proposed patch to improve an error message) and I was trying to
> figure out what to do with a bit of code in addEndpoint. Reading the
> code, I think I've spotted a problem:
>
> [187] UriTemplate uriTemplate = new UriTemplate(path);
> if (uriTemplate.hasParameters()) {
> Integer key = Integer.valueOf(uriTemplate.getSegmentCount());
> SortedSet<TemplatePathMatch> templateMatches =
> configTemplateMatchMap.get(key);
> if (templateMatches == null) {
> // Ensure that if concurrent threads execute this block they
> // both end up using the same TreeSet instance
> templateMatches = new TreeSet<>(
> TemplatePathMatchComparator.getInstance());
> configTemplateMatchMap.putIfAbsent(key, templateMatches);
> templateMatches = configTemplateMatchMap.get(key);
> }
> [200] if (!templateMatches.add(new TemplatePathMatch(sec,
> uriTemplate))) {
> // Duplicate uriTemplate;
> throw new DeploymentException(
> sm.getString("serverContainer.duplicatePaths", path,
> sec.getEndpointClass(),
> sec.getEndpointClass()));
> }
> }
>
> The problem is on line 200, where we check to see if
> templateMatches.add() returns false (meaning that the object we added to
> the set replaced one that was already there).
>
> I don't believe this branch can /ever/ be reached because the
> TemplatePathMatch class doesn't override Object.equals(), thus no two
> objects of that type will ever equal each other. Therefore, a new
> TemplatePathMatch object will always be added to the set.
>
> I'm not sure what the implications are of multiple TemplatePathMatch
> objects being in that set, but it looks like it's bad (we throw an
> exception in that case), so someone with a better understanding of such
> things might want to take a look.
>
> (Also, if anyone would care to comment on how to get the "pre-existing"
> match for the error message -- which is different than current-trunk as
> it's what I'm working on -- I'd love some insight.)
>
> Thanks,
> -chris
>