Re: Change in behaviour of CompressionFilter
On 25/03/2019 02:10, Jonathan Valliere wrote: I’m going to look at this starting Tuesday. Emmanuel do you mind creating a branch for this Jira issue so I can track what you’re working on? The branch name is 'mina-write-request'. It's from MINA 2.1.
Re: Change in behaviour of CompressionFilter
On 25/03/2019 02:10, Jonathan Valliere wrote: I’m going to look at this starting Tuesday. Emmanuel do you mind creating a branch for this Jira issue so I can track what you’re working on? Sure
Re: Change in behaviour of CompressionFilter
I’m going to look at this starting Tuesday. Emmanuel do you mind creating
a branch for this Jira issue so I can track what you’re working on?
On Sun, Mar 24, 2019 at 3:54 AM Emmanuel Lécharny
wrote:
> Things are complicated :/
>
> Ok, after two days of investigation, my take is that the original sin
> was to use a IoBuffer to propagate a message across the filters.
> IoBuffers can be consumed (ie, when you read them, their position
> change) making it very complex not to abuse them when writing a filter.
>
> Typical problem arose in SslFilter, ProtocolCodecFilter, LoggingFilter,
> CompressionFilter (and probably some more). Lets see what happens for
> those filters.
>
> LoggingFilter: actually, nothing is logged for filterWrite ! That kind
> of 'fix' the issue, by ignoring it. OTOH, it's not consistent: we may
> want to know what is being written...
>
> SslFilter: it's pretty obvious that the incoming filter will be fully
> consumed, and a new buffer will be produced. The interesting thing is
> that the SslEngine use ByteBuffer to encrypt/decrypt data. Anyway, we
> consume the incoming buffer, no matter what, and it's a side effect.
>
> ProtocolCodecFilter: it also consume the incoming message, which can be
> any kind of data structure. It doesn't matter because the factory is
> application specific (or it's one of the on the shelf factories). The
> special case is when the incoming message is an IoBuffer (not really
> frequently the case...)
>
> CompressionFilter: it takes a IoBuffer as input and produce a new one,
> having consumed the incoming IoBuffer.
>
> etc, etc.
>
> So when we have fully written the message, any IoBuffer than have been
> created in the filters - or any IoBBuffer that was passed by the
> application - would have been consumed. If we want to propagate a
> messageSent event, we need to have kept a track of the original message,
> and if it was a IoBuffer, that would mean duplicating it, or flip it
> before calling messageSent() just after having written it fully...
>
> Now, why are we using IoBuffer at all ? For three reasons:
>
> - ByteBuffer are immutable. It's valuable to be able to 'grow' a
> IoBuffer, typically when we inflate a message
>
> - SslEngine is using ByteBuffer, IoBuffer encapsulate one
>
> - Ultimately, writing into a channel is done using a ByteBuffer, so not
> having to create one when calling write on a channel saves some copy.
> But this is really when we have an app that has no filter modifying the
> IoBuffer. A rare corner case.
>
> So my take is that using an IoBuffer is a premature optimization. Just
> because channels require ByteBuffer does not lmean we shiould use
> ByteBuffer acorss the filters, except for the SslBuffer - which should
> always be the first filter in the chain anyway. We should also always
> keep a track of the original message, unmodified, so that the
> messageSent eveent can be propagated properly. Obviously, if the
> original message is an IoBuffer, and is never modified by any filteer,
> duplicating it before sending it just because we want to keep it intact
> for the messageSent event is a waste, but OTOH, it's very unlikely to be
> a frequent use case...
>
> I'm currently working on implementing that, and so far, it's going well.
>
>
> I'll keep the list informed.
>
> On 22/03/2019 13:35, Emmanuel Lécharny wrote:
> > Ok, I have a version that works better with the CompressionFilter.
> >
> >
> > Here is what I have changed :
> >
> > - the session.write( ) method will inject the original
> > message into the WriteRequest
> >
> > - any filterWrite filter that is going to create a new version of the
> > message will store it in the same writeRequest, into a modifiedMessage
> > field. (The WriteRequest impl thus holds 2 versions of the message,
> > the odiginal one and the modified one)
> >
> > - when the modified message is written to the remote peer (it's now an
> > IoBuffer), messageSent will be called with the original message, or
> > with the written IoBuffer flipped if the original message was a
> > IoBuffer and was never modified.
> >
> > The first test I did with the chat example seems to work pretty well.
> > It's going through a TextLineCodecFactory codec filter, a
> > CompressionFilter. I have to test it with the LoginFilter and the
> > SslFilter.
> >
> >
> > All in all, the MINA code is much simpler and should be faster too, as
> > we spare spurious calls to messageSent and filterWrite with an Empty
> > message.
> >
> >
> > More to come soon.
> >
> > On 22/03/2019 04:58, Emmanuel Lécharny wrote:
> >> Hmmpp
> >>
> >> I have traced the calls when a session.write( ) is done.
> >>
> >>
> >> It's all a kind of a hack.
> >>
> >>
> >> In order to be able to send the messageSent() event, the
> >> protocolFilter will call the nextFilter.filterWrite() method twice :
> >>
> >> public void filterWrite(NextFilter nextFilter, IoSession session,
> >> WriteRequest writeRequest) throws Exception {
> >> Object
Re: Change in behaviour of CompressionFilter
Things are complicated :/
Ok, after two days of investigation, my take is that the original sin
was to use a IoBuffer to propagate a message across the filters.
IoBuffers can be consumed (ie, when you read them, their position
change) making it very complex not to abuse them when writing a filter.
Typical problem arose in SslFilter, ProtocolCodecFilter, LoggingFilter,
CompressionFilter (and probably some more). Lets see what happens for
those filters.
LoggingFilter: actually, nothing is logged for filterWrite ! That kind
of 'fix' the issue, by ignoring it. OTOH, it's not consistent: we may
want to know what is being written...
SslFilter: it's pretty obvious that the incoming filter will be fully
consumed, and a new buffer will be produced. The interesting thing is
that the SslEngine use ByteBuffer to encrypt/decrypt data. Anyway, we
consume the incoming buffer, no matter what, and it's a side effect.
ProtocolCodecFilter: it also consume the incoming message, which can be
any kind of data structure. It doesn't matter because the factory is
application specific (or it's one of the on the shelf factories). The
special case is when the incoming message is an IoBuffer (not really
frequently the case...)
CompressionFilter: it takes a IoBuffer as input and produce a new one,
having consumed the incoming IoBuffer.
etc, etc.
So when we have fully written the message, any IoBuffer than have been
created in the filters - or any IoBBuffer that was passed by the
application - would have been consumed. If we want to propagate a
messageSent event, we need to have kept a track of the original message,
and if it was a IoBuffer, that would mean duplicating it, or flip it
before calling messageSent() just after having written it fully...
Now, why are we using IoBuffer at all ? For three reasons:
- ByteBuffer are immutable. It's valuable to be able to 'grow' a
IoBuffer, typically when we inflate a message
- SslEngine is using ByteBuffer, IoBuffer encapsulate one
- Ultimately, writing into a channel is done using a ByteBuffer, so not
having to create one when calling write on a channel saves some copy.
But this is really when we have an app that has no filter modifying the
IoBuffer. A rare corner case.
So my take is that using an IoBuffer is a premature optimization. Just
because channels require ByteBuffer does not lmean we shiould use
ByteBuffer acorss the filters, except for the SslBuffer - which should
always be the first filter in the chain anyway. We should also always
keep a track of the original message, unmodified, so that the
messageSent eveent can be propagated properly. Obviously, if the
original message is an IoBuffer, and is never modified by any filteer,
duplicating it before sending it just because we want to keep it intact
for the messageSent event is a waste, but OTOH, it's very unlikely to be
a frequent use case...
I'm currently working on implementing that, and so far, it's going well.
I'll keep the list informed.
On 22/03/2019 13:35, Emmanuel Lécharny wrote:
Ok, I have a version that works better with the CompressionFilter.
Here is what I have changed :
- the session.write( ) method will inject the original
message into the WriteRequest
- any filterWrite filter that is going to create a new version of the
message will store it in the same writeRequest, into a modifiedMessage
field. (The WriteRequest impl thus holds 2 versions of the message,
the odiginal one and the modified one)
- when the modified message is written to the remote peer (it's now an
IoBuffer), messageSent will be called with the original message, or
with the written IoBuffer flipped if the original message was a
IoBuffer and was never modified.
The first test I did with the chat example seems to work pretty well.
It's going through a TextLineCodecFactory codec filter, a
CompressionFilter. I have to test it with the LoginFilter and the
SslFilter.
All in all, the MINA code is much simpler and should be faster too, as
we spare spurious calls to messageSent and filterWrite with an Empty
message.
More to come soon.
On 22/03/2019 04:58, Emmanuel Lécharny wrote:
Hmmpp
I have traced the calls when a session.write( ) is done.
It's all a kind of a hack.
In order to be able to send the messageSent() event, the
protocolFilter will call the nextFilter.filterWrite() method twice :
public void filterWrite(NextFilter nextFilter, IoSession session,
WriteRequest writeRequest) throws Exception {
Object message = writeRequest.getMessage();
...
// Write all the encoded messages now
while (!bufferQueue.isEmpty()) {
Object encodedMessage = bufferQueue.poll();
// Flush only when the buffer has remaining.
if (!(encodedMessage instanceof IoBuffer) ||
((IoBuffer) encodedMessage).hasRemaining()) {
SocketAddress destination =
