On 20/03/2019 16:13, Jonathan Valliere wrote:
I’ll look into not using mark reset there.
I agree that using mark and reset is a kind of a non sense. Let's go
back to the original requirement :
- when we call a session.write(message), we know the message will go
through multiple filters
- at some point, it will be loaded into a IoBuffer (if not injected as
is when the session.write method is called)
- this IoBuffer will be consumed when we write it to the remote peer
- eventually, we want the original message (whatever it was) to be sent
back to the IoHandler, for it to know that it was sent
This is the last requirement that is the root cause of the problem. It
requires that we keep the message as is to be sent back to the
IoHandler. When we call session.write(), we create a new instance of
DefaultWriteRequest() which hold the original message, and we except to
keep it unmodified until we get it back in the IoHandler.messageSent().
This is OK when the original message is not an IoBuufer (which is likely
to be the case), but when it's a IoBuffer, then we are in trouble...
In the current impl, the AbstractPollingIoProcessor.writeBuffer() method
is most certainly overdoing. We could assume that we don't need to reset
the IoBuffer before calling messageSent(), and expect the IoHandler who
called session.write( IoBuffer ) to *know* that the IoBuffer has been
consumed, up to it to reset the IoBuffer. That put back the burden of
dealing with IoBuffer consommation to the user, which frankly is not a
big deal, because you are not really supposed to pass a mutable message
to the session (most of the time, there will be some Protocol filter in
the chain that will transform the message anyway.
So here is my take :
- stop calling reset before calling messageSent
- fix the tests that may fail because of this change
Note that it may impact MINA users who expect the message to be reset,
when message is an IoBuffer, and when the IoHandler implements
messageSent().
Thoughts ?
On Wed, Mar 20, 2019 at 11:05 AM Emmanuel Lécharny <[email protected]>
wrote:
On 20/03/2019 15:30, Jonathan Valliere wrote:
The offending code is in AbstractPollingIoProcessor. Are you agreeing
that
AbstractPollingIoProcessor has no place modifying the buffer positions?
Removing buf.reset() would probably fix the problem.
The reason we reset the buffer is that we need to send it back to the
IoHandler in the messageSent event, and to make it readable from its
starting point.
If the app is sending a message containg "hello world", this string will
be put into a IoBuffer, which will be read when the data will get sent
to the remote peer, and the position will then change. The messageSent
event will send this IoBuffer back to the IoHandler, which will then be
incapable of telling the app the message "hello world" has been sent,
because the buffer has been exhausted by the peer write...
This is the reason why we reset the buffer *after* it has been fully sent.
