You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Paul Clare <pc...@campuscruiser.com> on 2012/03/27 21:55:22 UTC
BoshBackendSessionContext.requestExpired - InfiniteLoop
I have been struggling with an issue where more than occasionally the delayedResponseQueue will be filled with empty responses until the server eventually runs out of memory. The client is JSJac but I am not really focused on that implementation, I just want to try and protect the server from being over run by any client.
So here is what is happening:
Here are the RIDs
318 never made it through.
Last sent response = 316
Highest Read = 317
Current = 319
Window = 319
The coding of
requestExpired {
... ...
while(!requestsWindow.isEmpty() && requestsWindow.firstKey() <= req.getRid()) {
write0(boshHandler.getEmptyResponse());
}
Coupled with
write0 {
if (requestsWindow.isEmpty() || requestsWindow.firstKey() > highestReadRid) {
delayedResponseQueue.offer(response);
Can easily lead to an infinite loop writing empty responses to the delayedResponseQueue.
So in trying to fix this I am having trouble understanding why this would be done in a loop ?
My first instinct is to change the while loop to an if statement but I assume there is a good reason it was implemented this way, I just cannot see it.
Is is not sufficient to write a single empty response when the request expires in the window?
The spec is of very little help here so any thoughts would be greatly appreciated.
Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
Posted by Bernd Fondermann <bf...@brainlounge.de>.
On 06.04.12 03:06, Mike Mahoney wrote:
>
>> However the spec is also extremely vague when it comes to the connection
>> manager.
>>
>> The RIDs (317, 319) are within in the "window" as per the spec, 318 never
>> makes it to the server (again this is to be expected as per the spec).
>
> I agree, it is very vague. I don't understand what the proper behavior should be for a missing RID. The connection manager is suppose to keep the request in order before sending to the server, but how long should it wait to send a request if the previous RID was missing? Any thoughts/opinions on this anyone? Maybe I'm missing something in the spec.
>
>>
>> <snip>
>>
>> For now I just changed the WHILE loop to an IF statement and we have not run
>> out of memory since.
>
> We did the same thing and it seems to be working for us as well. Thanks for the advice.
>
I've unassigned VYSPER-304.
Mike, if you want to apply the fix, pls go ahead.
Bernd
Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
Posted by Mike Mahoney <mi...@thingworx.com>.
> However the spec is also extremely vague when it comes to the connection
> manager.
>
> The RIDs (317, 319) are within in the "window" as per the spec, 318 never
> makes it to the server (again this is to be expected as per the spec).
I agree, it is very vague. I don't understand what the proper behavior should be for a missing RID. The connection manager is suppose to keep the request in order before sending to the server, but how long should it wait to send a request if the previous RID was missing? Any thoughts/opinions on this anyone? Maybe I'm missing something in the spec.
>
> <snip>
>
> For now I just changed the WHILE loop to an IF statement and we have not run
> out of memory since.
We did the same thing and it seems to be working for us as well. Thanks for the advice.
>
> The delayed response queue will rise and fall normally and there do not
> appear to be any dropped stanzas.
>
> -paul
>
>
> -----Original Message-----
> From: Bernd Fondermann
> Sent: Wednesday, April 04, 2012 4:50 PM
> To: dev@mina.apache.org
> Subject: Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
>
> On 04.04.12 19:53, Mike Mahoney wrote:
>>
>>> I have been struggling with an issue where more than occasionally
>>> the delayedResponseQueue will be filled with empty responses until
>>> the server eventually runs out of memory. The client is JSJac but I
>>> am not really focused on that implementation, I just want to try
>>> and protect the server from being over run by any client.
>>
>> We're running into the same issue now using Strophe, so it probably
>> isn't due to the client.
> <snip>
>>
>> If an rid is skipped, as in the scenario you outlined, then the
>> highestReadRid won't be incremented to 319 and will remain as 317
>> forever. Which causes the firstKey to be greater than the
>> highestReadRid. That loop doesn't seem right to me.
>>
>> Thoughts?
>
> Yes. I think BoshBackedSessionContext needs a major refactoring. The
> whole logic around queuing is flawed and hard to understand
> (code-readability). Missing synchronized statements can cause even more
> trouble.
>
> Bernd
>
----------------------------------------
Mike Mahoney
OEM Application Specialist
ThingWorx
Office: (610) 594-6200 x817
Mobile: (585) 314-8592
Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
Posted by Paul Clare <pc...@campuscruiser.com>.
I do not think any sort of synchronization will help in this particular case
although I agree the implementation could use some more comments so it could
be improved upon.
However the spec is also extremely vague when it comes to the connection
manager.
The RIDs (317, 319) are within in the "window" as per the spec, 318 never
makes it to the server (again this is to be expected as per the spec).
(A disturbing point here is that all these requests are "empty" keep alives)
IF the continuation expires BEFORE Jetty has determined the socket has been
dropped the server will run out of memory every time because the loop is
tight and the state will never change.
I am still quite puzzled as to why this was done in the first place, there
does not seem to been a reason to write more than a single "keep-alive"
response from this Jetty upstream notification.
synchronized private void requestExpired(Continuation continuation) {
... ...
while (!requestsWindow.isEmpty() && requestsWindow.firstKey() <=
req.getRid()) {
write0(boshHandler.getEmptyResponse());
}
For now I just changed the WHILE loop to an IF statement and we have not run
out of memory since.
The delayed response queue will rise and fall normally and there do not
appear to be any dropped stanzas.
-paul
-----Original Message-----
From: Bernd Fondermann
Sent: Wednesday, April 04, 2012 4:50 PM
To: dev@mina.apache.org
Subject: Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
On 04.04.12 19:53, Mike Mahoney wrote:
>
>> I have been struggling with an issue where more than occasionally
>> the delayedResponseQueue will be filled with empty responses until
>> the server eventually runs out of memory. The client is JSJac but I
>> am not really focused on that implementation, I just want to try
>> and protect the server from being over run by any client.
>
> We're running into the same issue now using Strophe, so it probably
> isn't due to the client.
<snip>
>
> If an rid is skipped, as in the scenario you outlined, then the
> highestReadRid won't be incremented to 319 and will remain as 317
> forever. Which causes the firstKey to be greater than the
> highestReadRid. That loop doesn't seem right to me.
>
> Thoughts?
Yes. I think BoshBackedSessionContext needs a major refactoring. The
whole logic around queuing is flawed and hard to understand
(code-readability). Missing synchronized statements can cause even more
trouble.
Bernd
Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
Posted by Bernd Fondermann <bf...@brainlounge.de>.
On 04.04.12 19:53, Mike Mahoney wrote:
>
>> I have been struggling with an issue where more than occasionally
>> the delayedResponseQueue will be filled with empty responses until
>> the server eventually runs out of memory. The client is JSJac but I
>> am not really focused on that implementation, I just want to try
>> and protect the server from being over run by any client.
>
> We're running into the same issue now using Strophe, so it probably
> isn't due to the client.
<snip>
>
> If an rid is skipped, as in the scenario you outlined, then the
> highestReadRid won't be incremented to 319 and will remain as 317
> forever. Which causes the firstKey to be greater than the
> highestReadRid. That loop doesn't seem right to me.
>
> Thoughts?
Yes. I think BoshBackedSessionContext needs a major refactoring. The
whole logic around queuing is flawed and hard to understand
(code-readability). Missing synchronized statements can cause even more
trouble.
Bernd
Re: BoshBackendSessionContext.requestExpired - InfiniteLoop
Posted by Mike Mahoney <mi...@thingworx.com>.
> I have been struggling with an issue where more than occasionally the delayedResponseQueue will be filled with empty responses until the server eventually runs out of memory. The client is JSJac but I am not really focused on that implementation, I just want to try and protect the server from being over run by any client.
We're running into the same issue now using Strophe, so it probably isn't due to the client.
> So here is what is happening:
>
> Here are the RIDs
>
> 318 never made it through.
>
> Last sent response = 316
> Highest Read = 317
> Current = 319
> Window = 319
>
> The coding of
>
> requestExpired {
> ... ...
>
> while(!requestsWindow.isEmpty() && requestsWindow.firstKey() <= req.getRid()) {
> write0(boshHandler.getEmptyResponse());
> }
>
> Coupled with
>
> write0 {
> if (requestsWindow.isEmpty() || requestsWindow.firstKey() > highestReadRid) {
> delayedResponseQueue.offer(response);
This if statement is the part I don't understand. It makes sense that you would queue the stanza for delivery if the requestsWindow is empty, but why is it queued when the first request in the requestsWindow has a rid greater than the highestReadRid. Going through the code, I can see how the firstKey could be higher than the highestReadRid, but I don't understand why it is allowed to be possible.
Specifically, looking at this for loop in BoshBackedSessionContext.insertRequest:
for (;;) {
// update the highestReadRid to the latest value
// it is possible to have higher RIDs than the highestReadRid with a gap between them (e.g. lost client request)
if (requestsWindow.containsKey(highestReadRid + 1)) {
highestReadRid++;
} else {
break;
}
}
If an rid is skipped, as in the scenario you outlined, then the highestReadRid won't be incremented to 319 and will remain as 317 forever. Which causes the firstKey to be greater than the highestReadRid. That loop doesn't seem right to me.
Thoughts?
> Can easily lead to an infinite loop writing empty responses to the delayedResponseQueue.
>
> So in trying to fix this I am having trouble understanding why this would be done in a loop ?
>
> My first instinct is to change the while loop to an if statement but I assume there is a good reason it was implemented this way, I just cannot see it.
>
> Is is not sufficient to write a single empty response when the request expires in the window?
Good question.
>
> The spec is of very little help here so any thoughts would be greatly appreciated.