You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Mike Mahoney <mi...@thingworx.com> on 2012/03/07 05:12:20 UTC
[Vysper] feedback on a fix for Vysper-18
Hey Everyone,
I've been looking into Vysper-18, 'Recognize and act upon closing stream tag'. I think I have a fix for it, but I wanted to get some input before doing a commit. Here is what I'm doing:
1. I noticed that XMLParser successfully determines the end of the stream document on lines 482-484, and notifies its content handler.
2. The Parser's content handler, XMPPContentHandler, ignores the endDocument() call. Instead of just ignoring it, I modified the XMLElementListener interface to be as follows:
public interface XMLElementListener {
void element(XMLElement element);
void close();
boolean isClosed();
}
and implemented this updated interface in XMPPDecoder.MinaStanzaListener. I then gave the MinaStanzaListener a boolean closed field so it could track its state.
3. In XMPPDecoder.doDecode() I modified the end of the method as follows:
XMPPContentHandler contentHandler = (XMPPContentHandler) reader.getContentHandler();
XMLElementListener listener = new MinaStanzaListener(out);
contentHandler.setListener(listener);
reader.parse(in, CharsetUtil.UTF8_DECODER); // If parsing find the end of the document, then the listener's close() will be called.
if (listener.isClosed()) {
session.close(false);
return true;
} else {
// we have parsed what we got, invoke again when more data is available
return false;
}
The call to session.close() then triggers the sessionClosed() handler in XmppIoHandlerAdapter, which cleans up the Vysper SessionContext.
I'm particularly looking for any thoughts on the session closing. I'm not a Mina expert by any means, so I want to make sure the close in XMPPDecoder.doDecode() handles everything cleanly. I realize this doesn't handle the BOSH case, but I can tackle that next.
Thanks for any input,
Mike
----------------------------------------
Mike Mahoney
OEM Application Specialist
ThingWorx
Office: (610) 594-6200 x817
Mobile: (585) 314-8592
Re: [Vysper] feedback on a fix for Vysper-18
Posted by Mike Mahoney <mi...@thingworx.com>.
On Mar 13, 2012, at 12:02 PM, Bernd Fondermann wrote:
> On Mon Mar 12 23:06:10 2012, Mike Mahoney wrote:
>>
>> On Mar 10, 2012, at 6:45 PM, Niklas Gustavsson wrote:
>>
>>> On Wed, Mar 7, 2012 at 5:12 AM, Mike Mahoney<mi...@thingworx.com> wrote:
>>>> I've been looking into Vysper-18, 'Recognize and act upon closing stream tag'. I think I have a fix for it, but I wanted to get some input before doing a commit. Here is what I'm doing:
>>>>
>>>> 1. I noticed that XMLParser successfully determines the end of the stream document on lines 482-484, and notifies its content handler.
>>>>
>>>> 2. The Parser's content handler, XMPPContentHandler, ignores the endDocument() call. Instead of just ignoring it, I modified the XMLElementListener interface to be as follows:
>>>>
>>>> public interface XMLElementListener {
>>>> void element(XMLElement element);
>>>> void close();
>>>> boolean isClosed();
>>>> }
>>>>
>>>> and implemented this updated interface in XMPPDecoder.MinaStanzaListener. I then gave the MinaStanzaListener a boolean closed field so it could track its state.
>>>>
>>>> 3. In XMPPDecoder.doDecode() I modified the end of the method as follows:
>>>>
>>>> XMPPContentHandler contentHandler = (XMPPContentHandler) reader.getContentHandler();
>>>> XMLElementListener listener = new MinaStanzaListener(out);
>>>> contentHandler.setListener(listener);
>>>>
>>>> reader.parse(in, CharsetUtil.UTF8_DECODER); // If parsing find the end of the document, then the listener's close() will be called.
>>>>
>>>> if (listener.isClosed()) {
>>>> session.close(false);
>>>> return true;
>>>> } else {
>>>> // we have parsed what we got, invoke again when more data is available
>>>> return false;
>>>> }
>>>>
>>>> The call to session.close() then triggers the sessionClosed() handler in XmppIoHandlerAdapter, which cleans up the Vysper SessionContext.
>>>>
>>>> I'm particularly looking for any thoughts on the session closing. I'm not a Mina expert by any means, so I want to make sure the close in XMPPDecoder.doDecode() handles everything cleanly. I realize this doesn't handle the BOSH case, but I can tackle that next.
>>>
>>> I find it a bit weird to do session management in the XML decoding
>>> code. Instead, could we not send a special marker (similar to
>>> SslFilter.SESSION_SECURED) on the end of document. It could then be
>>> detected in XmppIoHandlerAdapter where the session close would make
>>> more sense I think. Do you think that would work?
>>>
>>
>>
>> This same thing occurred to me. My reasoning was that the end of document is tightly tied to the end of the Mina session, so ending the mina session when the end of the stream happens felt like it made sense. I have made some additional changes to propagate the fact that it is a 'clean' client close up to the XmppIoHandlerAdapter. I do think that doing the Vysper session cleanup in the XmppIoHandlerAdapter makes sense.
>>
>> BTW, we've been doing some scalability testing and this change makes a big difference. If you have many clients going offline at the same time you will see a lot of exceptions in the log, due to calls being made to the parser after it is closed.
>
> +1. great to see this basic XMPP feature finally being implemented.
> thanks, Mike.
>
> Don't hesistate to commit it, we're in Commit-Then-Review mode
>
> Bernd
>
Thanks Bernd. I'll try to get it committed this week.
Re: [Vysper] feedback on a fix for Vysper-18
Posted by Bernd Fondermann <bf...@brainlounge.de>.
On Mon Mar 12 23:06:10 2012, Mike Mahoney wrote:
>
> On Mar 10, 2012, at 6:45 PM, Niklas Gustavsson wrote:
>
>> On Wed, Mar 7, 2012 at 5:12 AM, Mike Mahoney<mi...@thingworx.com> wrote:
>>> I've been looking into Vysper-18, 'Recognize and act upon closing stream tag'. I think I have a fix for it, but I wanted to get some input before doing a commit. Here is what I'm doing:
>>>
>>> 1. I noticed that XMLParser successfully determines the end of the stream document on lines 482-484, and notifies its content handler.
>>>
>>> 2. The Parser's content handler, XMPPContentHandler, ignores the endDocument() call. Instead of just ignoring it, I modified the XMLElementListener interface to be as follows:
>>>
>>> public interface XMLElementListener {
>>> void element(XMLElement element);
>>> void close();
>>> boolean isClosed();
>>> }
>>>
>>> and implemented this updated interface in XMPPDecoder.MinaStanzaListener. I then gave the MinaStanzaListener a boolean closed field so it could track its state.
>>>
>>> 3. In XMPPDecoder.doDecode() I modified the end of the method as follows:
>>>
>>> XMPPContentHandler contentHandler = (XMPPContentHandler) reader.getContentHandler();
>>> XMLElementListener listener = new MinaStanzaListener(out);
>>> contentHandler.setListener(listener);
>>>
>>> reader.parse(in, CharsetUtil.UTF8_DECODER); // If parsing find the end of the document, then the listener's close() will be called.
>>>
>>> if (listener.isClosed()) {
>>> session.close(false);
>>> return true;
>>> } else {
>>> // we have parsed what we got, invoke again when more data is available
>>> return false;
>>> }
>>>
>>> The call to session.close() then triggers the sessionClosed() handler in XmppIoHandlerAdapter, which cleans up the Vysper SessionContext.
>>>
>>> I'm particularly looking for any thoughts on the session closing. I'm not a Mina expert by any means, so I want to make sure the close in XMPPDecoder.doDecode() handles everything cleanly. I realize this doesn't handle the BOSH case, but I can tackle that next.
>>
>> I find it a bit weird to do session management in the XML decoding
>> code. Instead, could we not send a special marker (similar to
>> SslFilter.SESSION_SECURED) on the end of document. It could then be
>> detected in XmppIoHandlerAdapter where the session close would make
>> more sense I think. Do you think that would work?
>>
>
>
> This same thing occurred to me. My reasoning was that the end of document is tightly tied to the end of the Mina session, so ending the mina session when the end of the stream happens felt like it made sense. I have made some additional changes to propagate the fact that it is a 'clean' client close up to the XmppIoHandlerAdapter. I do think that doing the Vysper session cleanup in the XmppIoHandlerAdapter makes sense.
>
> BTW, we've been doing some scalability testing and this change makes a big difference. If you have many clients going offline at the same time you will see a lot of exceptions in the log, due to calls being made to the parser after it is closed.
+1. great to see this basic XMPP feature finally being implemented.
thanks, Mike.
Don't hesistate to commit it, we're in Commit-Then-Review mode
Bernd
Re: [Vysper] feedback on a fix for Vysper-18
Posted by Mike Mahoney <mi...@thingworx.com>.
On Mar 10, 2012, at 6:45 PM, Niklas Gustavsson wrote:
> On Wed, Mar 7, 2012 at 5:12 AM, Mike Mahoney <mi...@thingworx.com> wrote:
>> I've been looking into Vysper-18, 'Recognize and act upon closing stream tag'. I think I have a fix for it, but I wanted to get some input before doing a commit. Here is what I'm doing:
>>
>> 1. I noticed that XMLParser successfully determines the end of the stream document on lines 482-484, and notifies its content handler.
>>
>> 2. The Parser's content handler, XMPPContentHandler, ignores the endDocument() call. Instead of just ignoring it, I modified the XMLElementListener interface to be as follows:
>>
>> public interface XMLElementListener {
>> void element(XMLElement element);
>> void close();
>> boolean isClosed();
>> }
>>
>> and implemented this updated interface in XMPPDecoder.MinaStanzaListener. I then gave the MinaStanzaListener a boolean closed field so it could track its state.
>>
>> 3. In XMPPDecoder.doDecode() I modified the end of the method as follows:
>>
>> XMPPContentHandler contentHandler = (XMPPContentHandler) reader.getContentHandler();
>> XMLElementListener listener = new MinaStanzaListener(out);
>> contentHandler.setListener(listener);
>>
>> reader.parse(in, CharsetUtil.UTF8_DECODER); // If parsing find the end of the document, then the listener's close() will be called.
>>
>> if (listener.isClosed()) {
>> session.close(false);
>> return true;
>> } else {
>> // we have parsed what we got, invoke again when more data is available
>> return false;
>> }
>>
>> The call to session.close() then triggers the sessionClosed() handler in XmppIoHandlerAdapter, which cleans up the Vysper SessionContext.
>>
>> I'm particularly looking for any thoughts on the session closing. I'm not a Mina expert by any means, so I want to make sure the close in XMPPDecoder.doDecode() handles everything cleanly. I realize this doesn't handle the BOSH case, but I can tackle that next.
>
> I find it a bit weird to do session management in the XML decoding
> code. Instead, could we not send a special marker (similar to
> SslFilter.SESSION_SECURED) on the end of document. It could then be
> detected in XmppIoHandlerAdapter where the session close would make
> more sense I think. Do you think that would work?
>
This same thing occurred to me. My reasoning was that the end of document is tightly tied to the end of the Mina session, so ending the mina session when the end of the stream happens felt like it made sense. I have made some additional changes to propagate the fact that it is a 'clean' client close up to the XmppIoHandlerAdapter. I do think that doing the Vysper session cleanup in the XmppIoHandlerAdapter makes sense.
BTW, we've been doing some scalability testing and this change makes a big difference. If you have many clients going offline at the same time you will see a lot of exceptions in the log, due to calls being made to the parser after it is closed.
Re: [Vysper] feedback on a fix for Vysper-18
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Wed, Mar 7, 2012 at 5:12 AM, Mike Mahoney <mi...@thingworx.com> wrote:
> I've been looking into Vysper-18, 'Recognize and act upon closing stream tag'. I think I have a fix for it, but I wanted to get some input before doing a commit. Here is what I'm doing:
>
> 1. I noticed that XMLParser successfully determines the end of the stream document on lines 482-484, and notifies its content handler.
>
> 2. The Parser's content handler, XMPPContentHandler, ignores the endDocument() call. Instead of just ignoring it, I modified the XMLElementListener interface to be as follows:
>
> public interface XMLElementListener {
> void element(XMLElement element);
> void close();
> boolean isClosed();
> }
>
> and implemented this updated interface in XMPPDecoder.MinaStanzaListener. I then gave the MinaStanzaListener a boolean closed field so it could track its state.
>
> 3. In XMPPDecoder.doDecode() I modified the end of the method as follows:
>
> XMPPContentHandler contentHandler = (XMPPContentHandler) reader.getContentHandler();
> XMLElementListener listener = new MinaStanzaListener(out);
> contentHandler.setListener(listener);
>
> reader.parse(in, CharsetUtil.UTF8_DECODER); // If parsing find the end of the document, then the listener's close() will be called.
>
> if (listener.isClosed()) {
> session.close(false);
> return true;
> } else {
> // we have parsed what we got, invoke again when more data is available
> return false;
> }
>
> The call to session.close() then triggers the sessionClosed() handler in XmppIoHandlerAdapter, which cleans up the Vysper SessionContext.
>
> I'm particularly looking for any thoughts on the session closing. I'm not a Mina expert by any means, so I want to make sure the close in XMPPDecoder.doDecode() handles everything cleanly. I realize this doesn't handle the BOSH case, but I can tackle that next.
I find it a bit weird to do session management in the XML decoding
code. Instead, could we not send a special marker (similar to
SslFilter.SESSION_SECURED) on the end of document. It could then be
detected in XmppIoHandlerAdapter where the session close would make
more sense I think. Do you think that would work?
/niklas