You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Violeta Georgieva <vi...@apache.org> on 2017/02/06 19:55:31 UTC
Read events suspend/resume logic in websocket impl to achieve backpressure
Hi,
Currently JSR356 provides possibility to add message handlers in order to
receive web socket
messages but there is no way to instruct the web socket implementation to
suspend for a while
the incoming messages (backpressure) so that the application is able to
process the already delivered messages.
The other web containers (Jetty, Undertow) supports such functionality so I
would like to introduce it in Tomcat.
Here [1] I prepared one possible implementation.
What do you think about this feature and the proposed implementation?
Regards,
Violeta
[1] https://github.com/violetagg/tomcat/commits/ws-suspend-resume
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-02-07 9:55 GMT+02:00 Martin Grigorov <mg...@apache.org>:
>
> Hi Violeta,
>
> On Mon, Feb 6, 2017 at 8:55 PM, Violeta Georgieva <vi...@apache.org>
> wrote:
>
> > Hi,
> >
> > Currently JSR356 provides possibility to add message handlers in order
to
> > receive web socket
> > messages but there is no way to instruct the web socket implementation
to
> > suspend for a while
> > the incoming messages (backpressure) so that the application is able to
> > process the already delivered messages.
> > The other web containers (Jetty, Undertow) supports such functionality
so I
> > would like to introduce it in Tomcat.
> > Here [1] I prepared one possible implementation.
> >
> > What do you think about this feature and the proposed implementation?
> >
>
> It think it would be better to create a Pull Request even if the work is
> not complete.
> This way others will be able to comment on the changes and everyone here
at
> dev@ will see the comments.
> At the moment it is possible to comment on your commits (in your repo) but
> then only you will receive the feedback.
> With [2] it is much easier to see the whole diff but it is not possible to
> comment on it.
Here is the PR
https://github.com/apache/tomcat/pull/42
Regards,
Violeta
>
> >
> > Regards,
> > Violeta
> > [1] https://github.com/violetagg/tomcat/commits/ws-suspend-resume
> >
>
> [2]
>
https://github.com/apache/tomcat/compare/trunk...violetagg:ws-suspend-resume
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Martin Grigorov <mg...@apache.org>.
Hi Violeta,
On Mon, Feb 6, 2017 at 8:55 PM, Violeta Georgieva <vi...@apache.org>
wrote:
> Hi,
>
> Currently JSR356 provides possibility to add message handlers in order to
> receive web socket
> messages but there is no way to instruct the web socket implementation to
> suspend for a while
> the incoming messages (backpressure) so that the application is able to
> process the already delivered messages.
> The other web containers (Jetty, Undertow) supports such functionality so I
> would like to introduce it in Tomcat.
> Here [1] I prepared one possible implementation.
>
> What do you think about this feature and the proposed implementation?
>
It think it would be better to create a Pull Request even if the work is
not complete.
This way others will be able to comment on the changes and everyone here at
dev@ will see the comments.
At the moment it is possible to comment on your commits (in your repo) but
then only you will receive the feedback.
With [2] it is much easier to see the whole diff but it is not possible to
comment on it.
>
> Regards,
> Violeta
> [1] https://github.com/violetagg/tomcat/commits/ws-suspend-resume
>
[2]
https://github.com/apache/tomcat/compare/trunk...violetagg:ws-suspend-resume
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Rémy Maucherat <re...@apache.org>.
2017-02-08 0:51 GMT+01:00 Mark Thomas <ma...@apache.org>:
> On 06/02/17 19:55, Violeta Georgieva wrote:
>
>> Hi,
>>
>> Currently JSR356 provides possibility to add message handlers in order to
>> receive web socket
>> messages but there is no way to instruct the web socket implementation to
>> suspend for a while
>> the incoming messages (backpressure) so that the application is able to
>> process the already delivered messages.
>> The other web containers (Jetty, Undertow) supports such functionality so
>> I
>> would like to introduce it in Tomcat.
>> Here [1] I prepared one possible implementation.
>>
>> What do you think about this feature and the proposed implementation?
>>
>
> I suggest you go ahead and commit (and back-port) the formatting updates.
> They all look good and getting those out of the way will make the diff
> easier to read.
>
> I'm currently undecided on this.
>
> I understand the requirement but rather than have proprietary methods
> added to various WebSocket implementations, I would have preferred to see a
> reactive wrapper provided for Java WebSocket that would have used
> Server->Client WebSocket messages to communicate back pressure to the
> client.
>
> However, that doesn't work if the aim is to feed 'uncontrolled' WebSocket
> clients into a reactive server side framework. Blocking is going to be only
> option to apply back-pressure and better to do that just on the client
> rather than on the client and the server - which means this feature is
> required in some form.
>
> I guess that makes me reluctantly in favour of it in principle but I'd
> very much prefer to review a patch proposal minus the reformatting.
>
> This new solution says that it should be the network that takes care of
the congestion. Personally, I prefer thinking the application should
decouple its IO from the actual processing and take care of the backlog
itself. One example: with some APIs, we've noticed that when a server
disconnects or become unresponsive, they become notified with an error like
... Never. Or, even better, as you say, the server should send something to
the client to actually give it information, but it's all proprietary then
(client + server).
Overall, I don't really see the point of retrofitting this new stuff into
already old things like websockets, while HTTP/2 should have this server ->
client "control" capability (minus an actual API to use it from Servlets, I
guess).
If this is still included, I suppose you will make sure the performance
doesn't go down BTW.
Rémy
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Rémy Maucherat <re...@apache.org>.
2017-03-20 15:41 GMT+01:00 Violeta Georgieva <mi...@gmail.com>:
> Hi,
>
> 2017-02-27 16:50 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >
> > On 27/02/17 11:55, Violeta Georgieva wrote:
> >
> > <snip/>
> >
> > >> A new patch is available based on the provided comments.
> > >> Can you please review it.
> > >
> > > Any feedback for the latest changes
> >
> > Sorry for the delay.
> >
> > On a minor/style point, I'd prefer SUSPENDED rather than READ_SUSPENDED
> > since the socket won't be eligible for read or write.
>
> Ok
>
> > Thinking some more about that, could that cause problems? Does the patch
> > need to ensure write operations aren't attempted? Non-blocking writes
> > should be OK but a blocking write would be problematic.
>
> I was thinking something
> around org.apache.tomcat.websocket.WsRemoteEndpointBasic.send* methods.
> If the reading is suspended then these methods will do nothing and log
> error.
> What do you think?
>
> > I think there is still a timing / concurrency issue around resume().
> > Consider the following sequence:
> > - incoming message is being processed in WsFrameServer
> > - suspend() is called on another thread
> > - while loop ends and onDataAvailable returns
> > - resume() is called on another thread
> > - then WsHttpUpgradeHandler checks if the thread is suspended
> >
> > The problem is between onDataAvailable returning and
> > WsHttpUpgradeHandler checking if the thread is suspended. If resume() is
> > called and processed during that admittedly narrow gap, the socket will
> > end up in the Poller twice which - from past experience - will cause
> > problems.
>
> I fixed that. The PR https://github.com/apache/tomcat/pull/42 is updated.
>
> A long time ago there were two extensions proposed for the "comet" API
(still in the wiki somehow: https://wiki.apache.org/tomcat/WhatIsComet ).
As part of them was suspend/resume which do exactly what you propose:
suspend read events for a while. While the Servlet API additions also looks
very similar, it decided to be less flexible and in particular there's no
suspend/resume (conceptually with the Servlet API, you'd have to unset the
listeners which isn't allowed).
So, having looked at the history, it's hard for me to really complain about
this being added.
Rémy
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
2017-04-27 23:33 GMT+03:00 Mark Thomas <ma...@apache.org>:
>
> On 27/04/17 21:22, Violeta Georgieva wrote:
> > Hi,
> >
> > 2017-04-26 17:43 GMT+03:00 Mark Thomas <ma...@apache.org>:
> >>
> >> On 25/04/17 11:47, Violeta Georgieva wrote:
> >>
> >> <snip/>
> >>
> >>> Thanks for the review.
> >>> Changes for all comments are applied to the PR.
> >>> Can you take a look?
> >>
> >> Sure. A few more comments but nothing serious. Unless the fixes for any
> >> of these require large changes to the patch I'd be +1 on applying the
> >> patch with these fixes. I'd be fine with the patch being committed
> >> without the minor issues fixed as long as they were addressed later.
> >>
> >> Mark
> >>
> >>
> >> Moderate
> >>
> >> - If another thread calls suspend() after the call to close() it looks
> >> like there could be an issue. Is another state - CLOSING - required?
> >>
> >> - On the client READ means a read is progress and READY means data has
> >> been read and is being processed. On the server the meanings are
> >> reversed.
> >
> > You are correct and it is tricky to find fitting state names as the
server
> > and the client has different roles.
> > Currently the states mean:
> >
> > On the Server
> > - READY means we are waiting for a notification that data is ready to be
> > read from the socket
> > - READ means we are reading from the socket and processing data
> >
> > On the Client
> > - READ means that we will process the data if such has already been read
> > and more data will be read from the socket
> > - READY means data has been read and is available for processing
>
> Maybe just document the above in the Javadoc for the state diagram.
I think that WAITING/PROCESSING describe the states better than READY/READ
and I'm considering to change them.
> Mark
>
>
> >
> > What about to rename
> > READY -> WAITING which will have meaning:
> > - on the Server - waiting to read a data from the socket
> > - on the Client - waiting for a data to be processed
> >
> > READ -> PROCESSING
> > - on the Server - the data is read from the socket and processed
> > - on the Client - the available data is processed and more data is read
> > from the socket
> >
> > Also the other states will be:
> >
> > READY_SUSPENDING -> SUSPENDING_WAIT
> > READ_SUSPENDING -> SUSPENDING_PROCESS
> >
> > Regards,
> > Violeta
> >
> >>
> >> - A couple of lines have trailing whitespace
> >> (only moderate because it will break the CI system)
> >>
> >>
> >> Minor
> >>
> >> - The Javadoc for the state diagram would be clearer with separate
> >> lines for each transition rather than some lines being bi-directional
> >>
> >> - Can WsFrameClient.processSocketRead() be simplified? The try/catch
> >> block that sets read state to READY looks to be unnecessary. The code
> >> paths all appear to lead to close - and that sets the read state
> >> anyway.
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> >> For additional commands, e-mail: dev-help@tomcat.apache.org
> >>
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 27/04/17 21:22, Violeta Georgieva wrote:
> Hi,
>
> 2017-04-26 17:43 GMT+03:00 Mark Thomas <ma...@apache.org>:
>>
>> On 25/04/17 11:47, Violeta Georgieva wrote:
>>
>> <snip/>
>>
>>> Thanks for the review.
>>> Changes for all comments are applied to the PR.
>>> Can you take a look?
>>
>> Sure. A few more comments but nothing serious. Unless the fixes for any
>> of these require large changes to the patch I'd be +1 on applying the
>> patch with these fixes. I'd be fine with the patch being committed
>> without the minor issues fixed as long as they were addressed later.
>>
>> Mark
>>
>>
>> Moderate
>>
>> - If another thread calls suspend() after the call to close() it looks
>> like there could be an issue. Is another state - CLOSING - required?
>>
>> - On the client READ means a read is progress and READY means data has
>> been read and is being processed. On the server the meanings are
>> reversed.
>
> You are correct and it is tricky to find fitting state names as the server
> and the client has different roles.
> Currently the states mean:
>
> On the Server
> - READY means we are waiting for a notification that data is ready to be
> read from the socket
> - READ means we are reading from the socket and processing data
>
> On the Client
> - READ means that we will process the data if such has already been read
> and more data will be read from the socket
> - READY means data has been read and is available for processing
Maybe just document the above in the Javadoc for the state diagram.
Mark
>
> What about to rename
> READY -> WAITING which will have meaning:
> - on the Server - waiting to read a data from the socket
> - on the Client - waiting for a data to be processed
>
> READ -> PROCESSING
> - on the Server - the data is read from the socket and processed
> - on the Client - the available data is processed and more data is read
> from the socket
>
> Also the other states will be:
>
> READY_SUSPENDING -> SUSPENDING_WAIT
> READ_SUSPENDING -> SUSPENDING_PROCESS
>
> Regards,
> Violeta
>
>>
>> - A couple of lines have trailing whitespace
>> (only moderate because it will break the CI system)
>>
>>
>> Minor
>>
>> - The Javadoc for the state diagram would be clearer with separate
>> lines for each transition rather than some lines being bi-directional
>>
>> - Can WsFrameClient.processSocketRead() be simplified? The try/catch
>> block that sets read state to READY looks to be unnecessary. The code
>> paths all appear to lead to close - and that sets the read state
>> anyway.
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: why has some field not been locked before accessed.
Posted by Mark Thomas <ma...@apache.org>.
On 27/04/17 13:19, wrote:
> Hi,
> The field sessionCounter and rejectedSessions in org.apache.catalina.session.ManagerBase
> is not atomic field
>
>
> Now, These fields ware not locked before they were accessed in Concurrent environment.
>
>
> it was allowed because the very low probability ?
Yes. Performance trumps accuracy in those cases.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-04-28 9:54 GMT+03:00 Violeta Georgieva <vi...@apache.org>:
>
> Hi,
>
>> ...
>> ...
>
> The fixes for all comments are available in the PR.
> If there are no other comments I'm going to commit this functionality to
Tomcat 9.
The fix is in Tomcat 9 for a month now and there are no issues so far.
I'm planning to backport it to Tomcat 8.5 as we agreed.
> Thanks,
> Violeta
>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-04-26 17:43 GMT+03:00 Mark Thomas <ma...@apache.org>:
>
> On 25/04/17 11:47, Violeta Georgieva wrote:
>
> <snip/>
>
> > Thanks for the review.
> > Changes for all comments are applied to the PR.
> > Can you take a look?
>
> Sure. A few more comments but nothing serious. Unless the fixes for any
> of these require large changes to the patch I'd be +1 on applying the
> patch with these fixes. I'd be fine with the patch being committed
> without the minor issues fixed as long as they were addressed later.
>
> Mark
>
>
> Moderate
>
> - If another thread calls suspend() after the call to close() it looks
> like there could be an issue. Is another state - CLOSING - required?
>
> - On the client READ means a read is progress and READY means data has
> been read and is being processed. On the server the meanings are
> reversed.
>
> - A couple of lines have trailing whitespace
> (only moderate because it will break the CI system)
>
>
> Minor
>
> - The Javadoc for the state diagram would be clearer with separate
> lines for each transition rather than some lines being bi-directional
>
> - Can WsFrameClient.processSocketRead() be simplified? The try/catch
> block that sets read state to READY looks to be unnecessary. The code
> paths all appear to lead to close - and that sets the read state
> anyway.
The fixes for all comments are available in the PR.
If there are no other comments I'm going to commit this functionality to
Tomcat 9.
Thanks,
Violeta
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
why has some field not been locked before accessed.
Posted by 郭建军 <ji...@dev.bessystem.com>.
Hi,
The field sessionCounter and rejectedSessions in org.apache.catalina.session.ManagerBase
is not atomic field。
Now, These fields ware not locked before they were accessed in Concurrent environment.
it was allowed because the very low probability ?
Look forward to your reply, thanks very much!
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-04-26 17:43 GMT+03:00 Mark Thomas <ma...@apache.org>:
>
> On 25/04/17 11:47, Violeta Georgieva wrote:
>
> <snip/>
>
> > Thanks for the review.
> > Changes for all comments are applied to the PR.
> > Can you take a look?
>
> Sure. A few more comments but nothing serious. Unless the fixes for any
> of these require large changes to the patch I'd be +1 on applying the
> patch with these fixes. I'd be fine with the patch being committed
> without the minor issues fixed as long as they were addressed later.
>
> Mark
>
>
> Moderate
>
> - If another thread calls suspend() after the call to close() it looks
> like there could be an issue. Is another state - CLOSING - required?
>
> - On the client READ means a read is progress and READY means data has
> been read and is being processed. On the server the meanings are
> reversed.
You are correct and it is tricky to find fitting state names as the server
and the client has different roles.
Currently the states mean:
On the Server
- READY means we are waiting for a notification that data is ready to be
read from the socket
- READ means we are reading from the socket and processing data
On the Client
- READ means that we will process the data if such has already been read
and more data will be read from the socket
- READY means data has been read and is available for processing
What about to rename
READY -> WAITING which will have meaning:
- on the Server - waiting to read a data from the socket
- on the Client - waiting for a data to be processed
READ -> PROCESSING
- on the Server - the data is read from the socket and processed
- on the Client - the available data is processed and more data is read
from the socket
Also the other states will be:
READY_SUSPENDING -> SUSPENDING_WAIT
READ_SUSPENDING -> SUSPENDING_PROCESS
Regards,
Violeta
>
> - A couple of lines have trailing whitespace
> (only moderate because it will break the CI system)
>
>
> Minor
>
> - The Javadoc for the state diagram would be clearer with separate
> lines for each transition rather than some lines being bi-directional
>
> - Can WsFrameClient.processSocketRead() be simplified? The try/catch
> block that sets read state to READY looks to be unnecessary. The code
> paths all appear to lead to close - and that sets the read state
> anyway.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 25/04/17 11:47, Violeta Georgieva wrote:
<snip/>
> Thanks for the review.
> Changes for all comments are applied to the PR.
> Can you take a look?
Sure. A few more comments but nothing serious. Unless the fixes for any
of these require large changes to the patch I'd be +1 on applying the
patch with these fixes. I'd be fine with the patch being committed
without the minor issues fixed as long as they were addressed later.
Mark
Moderate
- If another thread calls suspend() after the call to close() it looks
like there could be an issue. Is another state - CLOSING - required?
- On the client READ means a read is progress and READY means data has
been read and is being processed. On the server the meanings are
reversed.
- A couple of lines have trailing whitespace
(only moderate because it will break the CI system)
Minor
- The Javadoc for the state diagram would be clearer with separate
lines for each transition rather than some lines being bi-directional
- Can WsFrameClient.processSocketRead() be simplified? The try/catch
block that sets read state to READY looks to be unnecessary. The code
paths all appear to lead to close - and that sets the read state
anyway.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
2017-04-21 15:38 GMT+03:00 Mark Thomas <ma...@apache.org>:
>
> On 19/04/17 11:43, Violeta Georgieva wrote:
> > Hi,
> >
> > 2017-03-29 23:59 GMT+03:00 Mark Thomas <ma...@apache.org>:
>
> <snip/>
>
> >> I think the problem is that "reading in progress" and suspended are
> >> treated separately when it is the combination of the two that
represents
> >> the current state.
> >>
> >> If it were me, I'd probably look at adding a state machine (but that is
> >> just how I tend to approach these sorts of problems).
> >
> > You are right. I reworked the patch, the new implementation is using a
> > state machine. The changes are available here
> > https://github.com/apache/tomcat/pull/42
> >
> > Can you please take a look?
>
> I think the concurrency issues around state have been resolved. However,
> I do have some other review comments - one of which I think is an issue
> that needs to be fixed before the patch is applied.
>
> I think there is the possibility of data loss on the client. Consider
> the following sequence in WsFrameClient.processSocketRead():
> - Large read -> response is filled
> - processSocketRead() copies response to inputBuffer
> - external thread calls suspend()
> - inputBuffer is not processed
> - while loop exits
> - another read is performed
> - that read completes and fills response
> - external thread calls resume()
> - processSocketRead() copies some of response to inputBuffer
> can't copy all since inputBuffer is now full
> data is left in response
> - external thread calls suspend()
> - inputBuffer is not processed
> - while loop exits
> - response is cleared and data is lost
>
>
> Minor:
>
> 1. In suspend() and resume() consider checks of the current state in all
> branches of the switch statement to reduce the risk of incorrect state
> transitions. The scenarios I can think of where this would be a problem
> are somewhat contrived so only a minor concern.
>
> 2. Is the SuspendableMessageReceiver interface necessary? More a style /
> consistency question than anything else.
>
> 3. I think resumeProcessing() needs a short Javadoc to make clear that
> there may be data in the input buffer that needs to be processed on resume
Thanks for the review.
Changes for all comments are applied to the PR.
Can you take a look?
Violeta
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 19/04/17 11:43, Violeta Georgieva wrote:
> Hi,
>
> 2017-03-29 23:59 GMT+03:00 Mark Thomas <ma...@apache.org>:
<snip/>
>> I think the problem is that "reading in progress" and suspended are
>> treated separately when it is the combination of the two that represents
>> the current state.
>>
>> If it were me, I'd probably look at adding a state machine (but that is
>> just how I tend to approach these sorts of problems).
>
> You are right. I reworked the patch, the new implementation is using a
> state machine. The changes are available here
> https://github.com/apache/tomcat/pull/42
>
> Can you please take a look?
I think the concurrency issues around state have been resolved. However,
I do have some other review comments - one of which I think is an issue
that needs to be fixed before the patch is applied.
I think there is the possibility of data loss on the client. Consider
the following sequence in WsFrameClient.processSocketRead():
- Large read -> response is filled
- processSocketRead() copies response to inputBuffer
- external thread calls suspend()
- inputBuffer is not processed
- while loop exits
- another read is performed
- that read completes and fills response
- external thread calls resume()
- processSocketRead() copies some of response to inputBuffer
can't copy all since inputBuffer is now full
data is left in response
- external thread calls suspend()
- inputBuffer is not processed
- while loop exits
- response is cleared and data is lost
Minor:
1. In suspend() and resume() consider checks of the current state in all
branches of the switch statement to reduce the risk of incorrect state
transitions. The scenarios I can think of where this would be a problem
are somewhat contrived so only a minor concern.
2. Is the SuspendableMessageReceiver interface necessary? More a style /
consistency question than anything else.
3. I think resumeProcessing() needs a short Javadoc to make clear that
there may be data in the input buffer that needs to be processed on resume
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-03-29 23:59 GMT+03:00 Mark Thomas <ma...@apache.org>:
>
> On 29/03/17 08:02, Violeta Georgieva wrote:
> >> 2017-02-27 16:50 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
> <snip/>
>
> >>> Overall, I like the approach and would support apply a patch along
these
> >>> lines once the timing issues are resolved.
> >
> > Any feedback about the new changes?
>
> First of all, kudos for implementing what is potentially complex
> processing cleanly. It makes reviewing it much easier.
>
> I think the fix for the previous timing issue has introduced a different
> one. Consider the following scenario.
>
> - thread A is in onDataAvailable() processing data
> - thread B calls suspendonDataAvailable()
> - thread A exits onDataAvailable()
> - thread A enters the if statement and prepares to return SUSPENDED
> - thread B calls resume which is a NO-OP because the reading flag is set
> - thread A enters finally block and clears reading flag
> - thread A returns SUSPENDED
>
> and processing hangs.
>
> I think the problem is that "reading in progress" and suspended are
> treated separately when it is the combination of the two that represents
> the current state.
>
> If it were me, I'd probably look at adding a state machine (but that is
> just how I tend to approach these sorts of problems).
You are right. I reworked the patch, the new implementation is using a
state machine. The changes are available here
https://github.com/apache/tomcat/pull/42
Can you please take a look?
Thanks for the review,
Violeta
> Mark
>
>
> >
> > Thanks for the review,
> > Violeta
> >>
> >>> <snip/>
> >>>
> >>>>>> This approach would mean some internal API changes but that is fine
> > for
> >>>> 9.0.x and I don't see a problem with 8.5.x either. Whether this is
> >>>> back-ported to 8.0.x and 7.0.x is TBD. It also opens up the
> > possibility of
> >>>> being able to suspend/resume other protocols but I haven't thought a
> > great
> >>>> deal about how that might work.
> >>>>>
> >>>>> I need this functionality only for 9.0.x and 8.5.x.
> >>>
> >>> Then let's not back-port this any earlier than 8.5.x.
> >>>
> >>> Mark
> >>>
> >>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> >>> For additional commands, e-mail: dev-help@tomcat.apache.org
> >>>
> >>
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 29/03/17 08:02, Violeta Georgieva wrote:
>> 2017-02-27 16:50 GMT+02:00 Mark Thomas <ma...@apache.org>:
<snip/>
>>> Overall, I like the approach and would support apply a patch along these
>>> lines once the timing issues are resolved.
>
> Any feedback about the new changes?
First of all, kudos for implementing what is potentially complex
processing cleanly. It makes reviewing it much easier.
I think the fix for the previous timing issue has introduced a different
one. Consider the following scenario.
- thread A is in onDataAvailable() processing data
- thread B calls suspendonDataAvailable()
- thread A exits onDataAvailable()
- thread A enters the if statement and prepares to return SUSPENDED
- thread B calls resume which is a NO-OP because the reading flag is set
- thread A enters finally block and clears reading flag
- thread A returns SUSPENDED
and processing hangs.
I think the problem is that "reading in progress" and suspended are
treated separately when it is the combination of the two that represents
the current state.
If it were me, I'd probably look at adding a state machine (but that is
just how I tend to approach these sorts of problems).
Mark
>
> Thanks for the review,
> Violeta
>>
>>> <snip/>
>>>
>>>>>> This approach would mean some internal API changes but that is fine
> for
>>>> 9.0.x and I don't see a problem with 8.5.x either. Whether this is
>>>> back-ported to 8.0.x and 7.0.x is TBD. It also opens up the
> possibility of
>>>> being able to suspend/resume other protocols but I haven't thought a
> great
>>>> deal about how that might work.
>>>>>
>>>>> I need this functionality only for 9.0.x and 8.5.x.
>>>
>>> Then let's not back-port this any earlier than 8.5.x.
>>>
>>> Mark
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-03-20 16:41 GMT+02:00 Violeta Georgieva <mi...@gmail.com>:
>
> Hi,
>
> 2017-02-27 16:50 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >
> > On 27/02/17 11:55, Violeta Georgieva wrote:
> >
> > <snip/>
> >
> > >> A new patch is available based on the provided comments.
> > >> Can you please review it.
> > >
> > > Any feedback for the latest changes
> >
> > Sorry for the delay.
> >
> > On a minor/style point, I'd prefer SUSPENDED rather than READ_SUSPENDED
> > since the socket won't be eligible for read or write.
>
> Ok
>
> > Thinking some more about that, could that cause problems? Does the patch
> > need to ensure write operations aren't attempted? Non-blocking writes
> > should be OK but a blocking write would be problematic.
>
> I was thinking something around
org.apache.tomcat.websocket.WsRemoteEndpointBasic.send* methods.
> If the reading is suspended then these methods will do nothing and log
error.
> What do you think?
>
> > I think there is still a timing / concurrency issue around resume().
> > Consider the following sequence:
> > - incoming message is being processed in WsFrameServer
> > - suspend() is called on another thread
> > - while loop ends and onDataAvailable returns
> > - resume() is called on another thread
> > - then WsHttpUpgradeHandler checks if the thread is suspended
> >
> > The problem is between onDataAvailable returning and
> > WsHttpUpgradeHandler checking if the thread is suspended. If resume() is
> > called and processed during that admittedly narrow gap, the socket will
> > end up in the Poller twice which - from past experience - will cause
> > problems.
>
> I fixed that. The PR https://github.com/apache/tomcat/pull/42 is updated.
>
> > Overall, I like the approach and would support apply a patch along these
> > lines once the timing issues are resolved.
Any feedback about the new changes?
Thanks for the review,
Violeta
>
> > <snip/>
> >
> > >>> This approach would mean some internal API changes but that is fine
for
> > > 9.0.x and I don't see a problem with 8.5.x either. Whether this is
> > > back-ported to 8.0.x and 7.0.x is TBD. It also opens up the
possibility of
> > > being able to suspend/resume other protocols but I haven't thought a
great
> > > deal about how that might work.
> > >>
> > >> I need this functionality only for 9.0.x and 8.5.x.
> >
> > Then let's not back-port this any earlier than 8.5.x.
> >
> > Mark
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <mi...@gmail.com>.
Hi,
2017-02-27 16:50 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
> On 27/02/17 11:55, Violeta Georgieva wrote:
>
> <snip/>
>
> >> A new patch is available based on the provided comments.
> >> Can you please review it.
> >
> > Any feedback for the latest changes
>
> Sorry for the delay.
>
> On a minor/style point, I'd prefer SUSPENDED rather than READ_SUSPENDED
> since the socket won't be eligible for read or write.
Ok
> Thinking some more about that, could that cause problems? Does the patch
> need to ensure write operations aren't attempted? Non-blocking writes
> should be OK but a blocking write would be problematic.
I was thinking something
around org.apache.tomcat.websocket.WsRemoteEndpointBasic.send* methods.
If the reading is suspended then these methods will do nothing and log
error.
What do you think?
> I think there is still a timing / concurrency issue around resume().
> Consider the following sequence:
> - incoming message is being processed in WsFrameServer
> - suspend() is called on another thread
> - while loop ends and onDataAvailable returns
> - resume() is called on another thread
> - then WsHttpUpgradeHandler checks if the thread is suspended
>
> The problem is between onDataAvailable returning and
> WsHttpUpgradeHandler checking if the thread is suspended. If resume() is
> called and processed during that admittedly narrow gap, the socket will
> end up in the Poller twice which - from past experience - will cause
> problems.
I fixed that. The PR https://github.com/apache/tomcat/pull/42 is updated.
> Overall, I like the approach and would support apply a patch along these
> lines once the timing issues are resolved.
Thanks for the review,
Violeta
> <snip/>
>
> >>> This approach would mean some internal API changes but that is fine
for
> > 9.0.x and I don't see a problem with 8.5.x either. Whether this is
> > back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility
of
> > being able to suspend/resume other protocols but I haven't thought a
great
> > deal about how that might work.
> >>
> >> I need this functionality only for 9.0.x and 8.5.x.
>
> Then let's not back-port this any earlier than 8.5.x.
>
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 27/02/17 11:55, Violeta Georgieva wrote:
<snip/>
>> A new patch is available based on the provided comments.
>> Can you please review it.
>
> Any feedback for the latest changes
Sorry for the delay.
On a minor/style point, I'd prefer SUSPENDED rather than READ_SUSPENDED
since the socket won't be eligible for read or write.
Thinking some more about that, could that cause problems? Does the patch
need to ensure write operations aren't attempted? Non-blocking writes
should be OK but a blocking write would be problematic.
I think there is still a timing / concurrency issue around resume().
Consider the following sequence:
- incoming message is being processed in WsFrameServer
- suspend() is called on another thread
- while loop ends and onDataAvailable returns
- resume() is called on another thread
- then WsHttpUpgradeHandler checks if the thread is suspended
The problem is between onDataAvailable returning and
WsHttpUpgradeHandler checking if the thread is suspended. If resume() is
called and processed during that admittedly narrow gap, the socket will
end up in the Poller twice which - from past experience - will cause
problems.
Overall, I like the approach and would support apply a patch along these
lines once the timing issues are resolved.
<snip/>
>>> This approach would mean some internal API changes but that is fine for
> 9.0.x and I don't see a problem with 8.5.x either. Whether this is
> back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility of
> being able to suspend/resume other protocols but I haven't thought a great
> deal about how that might work.
>>
>> I need this functionality only for 9.0.x and 8.5.x.
Then let's not back-port this any earlier than 8.5.x.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <mi...@gmail.com>.
Hi,
2017-02-14 23:43 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
>
> Hi,
>
>
> 2017-02-10 11:07 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >
> > On 09/02/17 22:08, Violeta Georgieva wrote:
> >>
> >> 2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
> >>>
> >>> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >
> >
> > <snip/>
> >
> >>>> I guess that makes me reluctantly in favour of it in principle but
I'd
> >>
> >> very much prefer to review a patch proposal minus the reformatting.
> >>>>
> >>>>
> >>>
> >>> There is a new patch
> >>> - no formatting noise
> >>> - Martin's comments included
> >>
> >>
> >> There is a new patch:
> >> - With a fix for the Martin's comment (StringManager)
> >> - I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order
to
> >> minimize the memory usage
> >
> >
> > Thanks. Much easier to read.
> >
> > Having reviewed the patch, I'm concerned about thread-safety on resume.
I'll use NIO terminology but I believe the same issues apply to all three
connectors.
> >
> > Consider the case where the client is sending data as fast as it can.
> >
> > On suspension, the socket will be added to the poller. More data will
arrive, the socket will be processed, no data will be read (because
processing is suspended) and the socket will be added to the poller again.
I'm fairly sure (but haven't confirmed with a test) that when more data
arrives the poller will trigger socket processing again. This loop will
continue until the network buffers are full. (Even if I am wrong on the
poller firing again immediately, there is still a problem.)
> >
> > On resume, the backlog of data needs to be processed. As currently
implemented, this backlog will be processed on the thread that calls
resume(). That may be undesirable for several reasons:
> > - it might not be a container thread;
> > - processing the backlog may take time impacting on other work the
> > thread expects to do
> > - when the poller triggers socket processing again there could be
> > two threads processing the same socket (very bad)
> >
> > Therefore, I think resume needs to call
socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
> >
> > That will solve the concurrent threads processing the same socket
problem but it could cause another problem. When that container thread
completes, it will add the socket to the poller again. The problem is that
the socket will already have been added to the poller. Adding a socket to
the poller more than once has caused problems in the past.
> >
> > That brings me to the conclusion that a different approach is needed. I
think we need a new SocketState value SUSPEND. Currently returning UPGRADED
from upgradeDispatch() registers the socket for read. SUSPEND would
essentially be a NO-OP. When resume() is called, it would trigger a call to
socketWrapper.processSocket(SocketEvent.OPEN_READ, true) during which when
upgradeDispatch() completes it would return UPGRADE, adding the socket to
the poller and allowing processing to continue.
> >
>
> A new patch is available based on the provided comments.
> Can you please review it.
Any feedback for the latest changes
Thanks,
Violeta
> > This approach would mean some internal API changes but that is fine for
9.0.x and I don't see a problem with 8.5.x either. Whether this is
back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility of
being able to suspend/resume other protocols but I haven't thought a great
deal about how that might work.
>
> I need this functionality only for 9.0.x and 8.5.x.
>
> Thanks,
> Violeta
>
> >
> > Because I started thinking about thread-safety on resume, I haven't dug
into the patch in detail.
> >
> >
> > Mark
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 15/02/17 17:46, Christopher Schultz wrote:
> Violeta,
>
> On 2/14/17 4:43 PM, Violeta Georgieva wrote:
>> Hi,
>>
>> 2017-02-10 11:07 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>>
>>> On 09/02/17 22:08, Violeta Georgieva wrote:
>>>>
>>>> 2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
>>>>>
>>>>> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>>
>>>
>>> <snip/>
>>>
>>>>>> I guess that makes me reluctantly in favour of it in principle but I'd
>>>>
>>>> very much prefer to review a patch proposal minus the reformatting.
>>>>>>
>>>>>>
>>>>>
>>>>> There is a new patch
>>>>> - no formatting noise
>>>>> - Martin's comments included
>>>>
>>>>
>>>> There is a new patch:
>>>> - With a fix for the Martin's comment (StringManager)
>>>> - I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order to
>>>> minimize the memory usage
>>>
>>>
>>> Thanks. Much easier to read.
>>>
>>> Having reviewed the patch, I'm concerned about thread-safety on resume.
>> I'll use NIO terminology but I believe the same issues apply to all three
>> connectors.
>>>
>>> Consider the case where the client is sending data as fast as it can.
>>>
>>> On suspension, the socket will be added to the poller. More data will
>> arrive, the socket will be processed, no data will be read (because
>> processing is suspended) and the socket will be added to the poller again.
>> I'm fairly sure (but haven't confirmed with a test) that when more data
>> arrives the poller will trigger socket processing again. This loop will
>> continue until the network buffers are full. (Even if I am wrong on the
>> poller firing again immediately, there is still a problem.)
>>>
>>> On resume, the backlog of data needs to be processed. As currently
>> implemented, this backlog will be processed on the thread that calls
>> resume(). That may be undesirable for several reasons:
>>> - it might not be a container thread;
>>> - processing the backlog may take time impacting on other work the
>>> thread expects to do
>>> - when the poller triggers socket processing again there could be
>>> two threads processing the same socket (very bad)
>>>
>>> Therefore, I think resume needs to call
>> socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
>>>
>>> That will solve the concurrent threads processing the same socket problem
>> but it could cause another problem. When that container thread completes,
>> it will add the socket to the poller again. The problem is that the socket
>> will already have been added to the poller. Adding a socket to the poller
>> more than once has caused problems in the past.
>>>
>>> That brings me to the conclusion that a different approach is needed. I
>> think we need a new SocketState value SUSPEND. Currently returning UPGRADED
>> from upgradeDispatch() registers the socket for read. SUSPEND would
>> essentially be a NO-OP. When resume() is called, it would trigger a call to
>> socketWrapper.processSocket(SocketEvent.OPEN_READ, true) during which when
>> upgradeDispatch() completes it would return UPGRADE, adding the socket to
>> the poller and allowing processing to continue.
>>>
>>
>> A new patch is available based on the provided comments.
>> Can you please review it.
>>
>>> This approach would mean some internal API changes but that is fine for
>> 9.0.x and I don't see a problem with 8.5.x either. Whether this is
>> back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility of
>> being able to suspend/resume other protocols but I haven't thought a great
>> deal about how that might work.
>>
>> I need this functionality only for 9.0.x and 8.5.x.
>
> I think we need to come to an agreement about what it's okay to change
> in a non-backward-compatible way in terms of internal APIs. It's become
> clear lately that API breakages really affect users, and we shouldn't
> take that lightly.
Read the RELEASE_NOTES file at the top of the svn tree for any major
version.
Mark
>
> 8.5.x is already "stable" and users are actively being encouraged to
> move from 8.0 to it. I think that qualifies it as being something we
> should be very careful about changing.
>
> I'm not trying to say that APIs can never change; I just want to do it
> in a way that is minimally-breaking to downstream users.
>
> -chris
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Violeta,
On 2/15/17 1:14 PM, Violeta Georgieva wrote:
> 2017-02-15 19:46 GMT+02:00 Christopher Schultz <chris@christopherschultz.net
>> I think we need to come to an agreement about what it's okay to change
>> in a non-backward-compatible way in terms of internal APIs. It's become
>> clear lately that API breakages really affect users, and we shouldn't
>> take that lightly.
>>
>> 8.5.x is already "stable" and users are actively being encouraged to
>> move from 8.0 to it. I think that qualifies it as being something we
>> should be very careful about changing.
>>
>> I'm not trying to say that APIs can never change; I just want to do it
>> in a way that is minimally-breaking to downstream users.
>
> The change introduces a new API. Existing APIs are not modified at all and
> the change is fully backwards compatible.
> All Junit tests are passing and the results of the Web Socket Autobahn
> tests are the same as before the change.
> Do you have some concrete concerns or proposals regarding the
> implementation?
> I can test some concrete scenarios if you have some in mind.
I apologize if I raised the warning flag unnecessarily.
I admit that I hadn't read-through the patches very carefully, and I'm
not at all familiar with the byte-pushing code deep within Tomcat.
Introducing new APIs should not be a problem at all.
Thanks for the clarification. Apologies for the noise.
-chris
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi Chris,
2017-02-15 19:46 GMT+02:00 Christopher Schultz <chris@christopherschultz.net
>:
>
> Violeta,
>
> On 2/14/17 4:43 PM, Violeta Georgieva wrote:
> > Hi,
> >
> > 2017-02-10 11:07 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >>
> >> On 09/02/17 22:08, Violeta Georgieva wrote:
> >>>
> >>> 2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
> >>>>
> >>>> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >>
> >>
> >> <snip/>
> >>
> >>>>> I guess that makes me reluctantly in favour of it in principle but
I'd
> >>>
> >>> very much prefer to review a patch proposal minus the reformatting.
> >>>>>
> >>>>>
> >>>>
> >>>> There is a new patch
> >>>> - no formatting noise
> >>>> - Martin's comments included
> >>>
> >>>
> >>> There is a new patch:
> >>> - With a fix for the Martin's comment (StringManager)
> >>> - I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order
to
> >>> minimize the memory usage
> >>
> >>
> >> Thanks. Much easier to read.
> >>
> >> Having reviewed the patch, I'm concerned about thread-safety on resume.
> > I'll use NIO terminology but I believe the same issues apply to all
three
> > connectors.
> >>
> >> Consider the case where the client is sending data as fast as it can.
> >>
> >> On suspension, the socket will be added to the poller. More data will
> > arrive, the socket will be processed, no data will be read (because
> > processing is suspended) and the socket will be added to the poller
again.
> > I'm fairly sure (but haven't confirmed with a test) that when more data
> > arrives the poller will trigger socket processing again. This loop will
> > continue until the network buffers are full. (Even if I am wrong on the
> > poller firing again immediately, there is still a problem.)
> >>
> >> On resume, the backlog of data needs to be processed. As currently
> > implemented, this backlog will be processed on the thread that calls
> > resume(). That may be undesirable for several reasons:
> >> - it might not be a container thread;
> >> - processing the backlog may take time impacting on other work the
> >> thread expects to do
> >> - when the poller triggers socket processing again there could be
> >> two threads processing the same socket (very bad)
> >>
> >> Therefore, I think resume needs to call
> > socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
> >>
> >> That will solve the concurrent threads processing the same socket
problem
> > but it could cause another problem. When that container thread
completes,
> > it will add the socket to the poller again. The problem is that the
socket
> > will already have been added to the poller. Adding a socket to the
poller
> > more than once has caused problems in the past.
> >>
> >> That brings me to the conclusion that a different approach is needed. I
> > think we need a new SocketState value SUSPEND. Currently returning
UPGRADED
> > from upgradeDispatch() registers the socket for read. SUSPEND would
> > essentially be a NO-OP. When resume() is called, it would trigger a
call to
> > socketWrapper.processSocket(SocketEvent.OPEN_READ, true) during which
when
> > upgradeDispatch() completes it would return UPGRADE, adding the socket
to
> > the poller and allowing processing to continue.
> >>
> >
> > A new patch is available based on the provided comments.
> > Can you please review it.
> >
> >> This approach would mean some internal API changes but that is fine for
> > 9.0.x and I don't see a problem with 8.5.x either. Whether this is
> > back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility
of
> > being able to suspend/resume other protocols but I haven't thought a
great
> > deal about how that might work.
> >
> > I need this functionality only for 9.0.x and 8.5.x.
>
> I think we need to come to an agreement about what it's okay to change
> in a non-backward-compatible way in terms of internal APIs. It's become
> clear lately that API breakages really affect users, and we shouldn't
> take that lightly.
>
> 8.5.x is already "stable" and users are actively being encouraged to
> move from 8.0 to it. I think that qualifies it as being something we
> should be very careful about changing.
>
> I'm not trying to say that APIs can never change; I just want to do it
> in a way that is minimally-breaking to downstream users.
>
The change introduces a new API. Existing APIs are not modified at all and
the change is fully backwards compatible.
All Junit tests are passing and the results of the Web Socket Autobahn
tests are the same as before the change.
Do you have some concrete concerns or proposals regarding the
implementation?
I can test some concrete scenarios if you have some in mind.
Regards,
Violeta
> -chris
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Violeta,
On 2/14/17 4:43 PM, Violeta Georgieva wrote:
> Hi,
>
> 2017-02-10 11:07 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>
>> On 09/02/17 22:08, Violeta Georgieva wrote:
>>>
>>> 2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
>>>>
>>>> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>
>>
>> <snip/>
>>
>>>>> I guess that makes me reluctantly in favour of it in principle but I'd
>>>
>>> very much prefer to review a patch proposal minus the reformatting.
>>>>>
>>>>>
>>>>
>>>> There is a new patch
>>>> - no formatting noise
>>>> - Martin's comments included
>>>
>>>
>>> There is a new patch:
>>> - With a fix for the Martin's comment (StringManager)
>>> - I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order to
>>> minimize the memory usage
>>
>>
>> Thanks. Much easier to read.
>>
>> Having reviewed the patch, I'm concerned about thread-safety on resume.
> I'll use NIO terminology but I believe the same issues apply to all three
> connectors.
>>
>> Consider the case where the client is sending data as fast as it can.
>>
>> On suspension, the socket will be added to the poller. More data will
> arrive, the socket will be processed, no data will be read (because
> processing is suspended) and the socket will be added to the poller again.
> I'm fairly sure (but haven't confirmed with a test) that when more data
> arrives the poller will trigger socket processing again. This loop will
> continue until the network buffers are full. (Even if I am wrong on the
> poller firing again immediately, there is still a problem.)
>>
>> On resume, the backlog of data needs to be processed. As currently
> implemented, this backlog will be processed on the thread that calls
> resume(). That may be undesirable for several reasons:
>> - it might not be a container thread;
>> - processing the backlog may take time impacting on other work the
>> thread expects to do
>> - when the poller triggers socket processing again there could be
>> two threads processing the same socket (very bad)
>>
>> Therefore, I think resume needs to call
> socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
>>
>> That will solve the concurrent threads processing the same socket problem
> but it could cause another problem. When that container thread completes,
> it will add the socket to the poller again. The problem is that the socket
> will already have been added to the poller. Adding a socket to the poller
> more than once has caused problems in the past.
>>
>> That brings me to the conclusion that a different approach is needed. I
> think we need a new SocketState value SUSPEND. Currently returning UPGRADED
> from upgradeDispatch() registers the socket for read. SUSPEND would
> essentially be a NO-OP. When resume() is called, it would trigger a call to
> socketWrapper.processSocket(SocketEvent.OPEN_READ, true) during which when
> upgradeDispatch() completes it would return UPGRADE, adding the socket to
> the poller and allowing processing to continue.
>>
>
> A new patch is available based on the provided comments.
> Can you please review it.
>
>> This approach would mean some internal API changes but that is fine for
> 9.0.x and I don't see a problem with 8.5.x either. Whether this is
> back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility of
> being able to suspend/resume other protocols but I haven't thought a great
> deal about how that might work.
>
> I need this functionality only for 9.0.x and 8.5.x.
I think we need to come to an agreement about what it's okay to change
in a non-backward-compatible way in terms of internal APIs. It's become
clear lately that API breakages really affect users, and we shouldn't
take that lightly.
8.5.x is already "stable" and users are actively being encouraged to
move from 8.0 to it. I think that qualifies it as being something we
should be very careful about changing.
I'm not trying to say that APIs can never change; I just want to do it
in a way that is minimally-breaking to downstream users.
-chris
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-02-10 11:07 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
> On 09/02/17 22:08, Violeta Georgieva wrote:
>>
>> 2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
>>>
>>> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
>
> <snip/>
>
>>>> I guess that makes me reluctantly in favour of it in principle but I'd
>>
>> very much prefer to review a patch proposal minus the reformatting.
>>>>
>>>>
>>>
>>> There is a new patch
>>> - no formatting noise
>>> - Martin's comments included
>>
>>
>> There is a new patch:
>> - With a fix for the Martin's comment (StringManager)
>> - I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order to
>> minimize the memory usage
>
>
> Thanks. Much easier to read.
>
> Having reviewed the patch, I'm concerned about thread-safety on resume.
I'll use NIO terminology but I believe the same issues apply to all three
connectors.
>
> Consider the case where the client is sending data as fast as it can.
>
> On suspension, the socket will be added to the poller. More data will
arrive, the socket will be processed, no data will be read (because
processing is suspended) and the socket will be added to the poller again.
I'm fairly sure (but haven't confirmed with a test) that when more data
arrives the poller will trigger socket processing again. This loop will
continue until the network buffers are full. (Even if I am wrong on the
poller firing again immediately, there is still a problem.)
>
> On resume, the backlog of data needs to be processed. As currently
implemented, this backlog will be processed on the thread that calls
resume(). That may be undesirable for several reasons:
> - it might not be a container thread;
> - processing the backlog may take time impacting on other work the
> thread expects to do
> - when the poller triggers socket processing again there could be
> two threads processing the same socket (very bad)
>
> Therefore, I think resume needs to call
socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
>
> That will solve the concurrent threads processing the same socket problem
but it could cause another problem. When that container thread completes,
it will add the socket to the poller again. The problem is that the socket
will already have been added to the poller. Adding a socket to the poller
more than once has caused problems in the past.
>
> That brings me to the conclusion that a different approach is needed. I
think we need a new SocketState value SUSPEND. Currently returning UPGRADED
from upgradeDispatch() registers the socket for read. SUSPEND would
essentially be a NO-OP. When resume() is called, it would trigger a call to
socketWrapper.processSocket(SocketEvent.OPEN_READ, true) during which when
upgradeDispatch() completes it would return UPGRADE, adding the socket to
the poller and allowing processing to continue.
>
A new patch is available based on the provided comments.
Can you please review it.
> This approach would mean some internal API changes but that is fine for
9.0.x and I don't see a problem with 8.5.x either. Whether this is
back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility of
being able to suspend/resume other protocols but I haven't thought a great
deal about how that might work.
I need this functionality only for 9.0.x and 8.5.x.
Thanks,
Violeta
>
> Because I started thinking about thread-safety on resume, I haven't dug
into the patch in detail.
>
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 09/02/17 22:08, Violeta Georgieva wrote:
> 2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
>> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
<snip/>
>>> I guess that makes me reluctantly in favour of it in principle but I'd
> very much prefer to review a patch proposal minus the reformatting.
>>>
>>
>> There is a new patch
>> - no formatting noise
>> - Martin's comments included
>
> There is a new patch:
> - With a fix for the Martin's comment (StringManager)
> - I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order to
> minimize the memory usage
Thanks. Much easier to read.
Having reviewed the patch, I'm concerned about thread-safety on resume.
I'll use NIO terminology but I believe the same issues apply to all
three connectors.
Consider the case where the client is sending data as fast as it can.
On suspension, the socket will be added to the poller. More data will
arrive, the socket will be processed, no data will be read (because
processing is suspended) and the socket will be added to the poller
again. I'm fairly sure (but haven't confirmed with a test) that when
more data arrives the poller will trigger socket processing again. This
loop will continue until the network buffers are full. (Even if I am
wrong on the poller firing again immediately, there is still a problem.)
On resume, the backlog of data needs to be processed. As currently
implemented, this backlog will be processed on the thread that calls
resume(). That may be undesirable for several reasons:
- it might not be a container thread;
- processing the backlog may take time impacting on other work the
thread expects to do
- when the poller triggers socket processing again there could be
two threads processing the same socket (very bad)
Therefore, I think resume needs to call
socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
That will solve the concurrent threads processing the same socket
problem but it could cause another problem. When that container thread
completes, it will add the socket to the poller again. The problem is
that the socket will already have been added to the poller. Adding a
socket to the poller more than once has caused problems in the past.
That brings me to the conclusion that a different approach is needed. I
think we need a new SocketState value SUSPEND. Currently returning
UPGRADED from upgradeDispatch() registers the socket for read. SUSPEND
would essentially be a NO-OP. When resume() is called, it would trigger
a call to socketWrapper.processSocket(SocketEvent.OPEN_READ, true)
during which when upgradeDispatch() completes it would return UPGRADE,
adding the socket to the poller and allowing processing to continue.
This approach would mean some internal API changes but that is fine for
9.0.x and I don't see a problem with 8.5.x either. Whether this is
back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility
of being able to suspend/resume other protocols but I haven't thought a
great deal about how that might work.
Because I started thinking about thread-safety on resume, I haven't dug
into the patch in detail.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
Hi,
2017-02-08 11:25 GMT+02:00 Violeta Georgieva <vi...@apache.org>:
>
>
>
> 2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>
>> On 06/02/17 19:55, Violeta Georgieva wrote:
>>>
>>> Hi,
>>>
>>> Currently JSR356 provides possibility to add message handlers in order
to
>>> receive web socket
>>> messages but there is no way to instruct the web socket implementation
to
>>> suspend for a while
>>> the incoming messages (backpressure) so that the application is able to
>>> process the already delivered messages.
>>> The other web containers (Jetty, Undertow) supports such functionality
so I
>>> would like to introduce it in Tomcat.
>>> Here [1] I prepared one possible implementation.
>>>
>>> What do you think about this feature and the proposed implementation?
>>
>>
>> I suggest you go ahead and commit (and back-port) the formatting
updates. They all look good and getting those out of the way will make the
diff easier to read.
>
>
> Formatting changes were committed.
>
>>
>>
>> I'm currently undecided on this.
>>
>> I understand the requirement but rather than have proprietary methods
added to various WebSocket implementations, I would have preferred to see a
reactive wrapper provided for Java WebSocket that would have used
Server->Client WebSocket messages to communicate back pressure to the
client.
>>
>> However, that doesn't work if the aim is to feed 'uncontrolled'
WebSocket clients into a reactive server side framework. Blocking is going
to be only option to apply back-pressure and better to do that just on the
client rather than on the client and the server - which means this feature
is required in some form.
>>
>> I guess that makes me reluctantly in favour of it in principle but I'd
very much prefer to review a patch proposal minus the reformatting.
>>
>
> There is a new patch
> - no formatting noise
> - Martin's comments included
There is a new patch:
- With a fix for the Martin's comment (StringManager)
- I switched from AtomicBoolean to AtomicIntegerFieldUpdater in order to
minimize the memory usage
Regards,
Violeta
>
>>
>> Mark
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <vi...@apache.org>.
2017-02-08 1:51 GMT+02:00 Mark Thomas <ma...@apache.org>:
> On 06/02/17 19:55, Violeta Georgieva wrote:
>
>> Hi,
>>
>> Currently JSR356 provides possibility to add message handlers in order to
>> receive web socket
>> messages but there is no way to instruct the web socket implementation to
>> suspend for a while
>> the incoming messages (backpressure) so that the application is able to
>> process the already delivered messages.
>> The other web containers (Jetty, Undertow) supports such functionality so
>> I
>> would like to introduce it in Tomcat.
>> Here [1] I prepared one possible implementation.
>>
>> What do you think about this feature and the proposed implementation?
>>
>
> I suggest you go ahead and commit (and back-port) the formatting updates.
> They all look good and getting those out of the way will make the diff
> easier to read.
>
Formatting changes were committed.
>
> I'm currently undecided on this.
>
> I understand the requirement but rather than have proprietary methods
> added to various WebSocket implementations, I would have preferred to see a
> reactive wrapper provided for Java WebSocket that would have used
> Server->Client WebSocket messages to communicate back pressure to the
> client.
>
> However, that doesn't work if the aim is to feed 'uncontrolled' WebSocket
> clients into a reactive server side framework. Blocking is going to be only
> option to apply back-pressure and better to do that just on the client
> rather than on the client and the server - which means this feature is
> required in some form.
>
> I guess that makes me reluctantly in favour of it in principle but I'd
> very much prefer to review a patch proposal minus the reformatting.
>
>
There is a new patch
- no formatting noise
- Martin's comments included
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: Read events suspend/resume logic in websocket impl to achieve
backpressure
Posted by Mark Thomas <ma...@apache.org>.
On 06/02/17 19:55, Violeta Georgieva wrote:
> Hi,
>
> Currently JSR356 provides possibility to add message handlers in order to
> receive web socket
> messages but there is no way to instruct the web socket implementation to
> suspend for a while
> the incoming messages (backpressure) so that the application is able to
> process the already delivered messages.
> The other web containers (Jetty, Undertow) supports such functionality so I
> would like to introduce it in Tomcat.
> Here [1] I prepared one possible implementation.
>
> What do you think about this feature and the proposed implementation?
I suggest you go ahead and commit (and back-port) the formatting
updates. They all look good and getting those out of the way will make
the diff easier to read.
I'm currently undecided on this.
I understand the requirement but rather than have proprietary methods
added to various WebSocket implementations, I would have preferred to
see a reactive wrapper provided for Java WebSocket that would have used
Server->Client WebSocket messages to communicate back pressure to the
client.
However, that doesn't work if the aim is to feed 'uncontrolled'
WebSocket clients into a reactive server side framework. Blocking is
going to be only option to apply back-pressure and better to do that
just on the client rather than on the client and the server - which
means this feature is required in some form.
I guess that makes me reluctantly in favour of it in principle but I'd
very much prefer to review a patch proposal minus the reformatting.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Violeta Georgieva <mi...@gmail.com>.
Hi,
2017-02-07 11:54 GMT+02:00 Rémy Maucherat <re...@apache.org>:
>
> 2017-02-06 20:55 GMT+01:00 Violeta Georgieva <vi...@apache.org>:
>
> > Hi,
> >
> > Currently JSR356 provides possibility to add message handlers in order
to
> > receive web socket
> > messages but there is no way to instruct the web socket implementation
to
> > suspend for a while
> > the incoming messages (backpressure) so that the application is able to
> > process the already delivered messages.
> > The other web containers (Jetty, Undertow) supports such functionality
so I
> > would like to introduce it in Tomcat.
> > Here [1] I prepared one possible implementation.
> >
> > What do you think about this feature and the proposed implementation?
> >
>
> I don't understand why this is that useful (it has to be used in a smart
> way that improves scalability by the application, I'm not convinced this
> can happen) but more importantly it's a proprietary API.
This functionality is needed in order to provide a proper implementation of
reactive streams for Tomcat.
https://github.com/reactive-streams/reactive-streams-jvm
Imagine that the component that is consuming the messages cannot accept
anymore.
On one hand there is no way to tell Tomcat to stop sending messages, on the
other non-blocking is expected.
A solution would be to buffer the messages in the memory, but the buffering
cannot be done without limits.
So if there is an API to tell Tomcat to stop reading it will be better than
buffering in the memory, which easily can lead to out of memory issues.
As there is no a standard, the API is proprietary. But it is needed and
other servers also provided it.
For sure there are other use cases than the one I mentioned above.
Regards,
Violeta
> Rémy
>
> >
> > Regards,
> > Violeta
> > [1] https://github.com/violetagg/tomcat/commits/ws-suspend-resume
> >
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
Posted by Rémy Maucherat <re...@apache.org>.
2017-02-06 20:55 GMT+01:00 Violeta Georgieva <vi...@apache.org>:
> Hi,
>
> Currently JSR356 provides possibility to add message handlers in order to
> receive web socket
> messages but there is no way to instruct the web socket implementation to
> suspend for a while
> the incoming messages (backpressure) so that the application is able to
> process the already delivered messages.
> The other web containers (Jetty, Undertow) supports such functionality so I
> would like to introduce it in Tomcat.
> Here [1] I prepared one possible implementation.
>
> What do you think about this feature and the proposed implementation?
>
I don't understand why this is that useful (it has to be used in a smart
way that improves scalability by the application, I'm not convinced this
can happen) but more importantly it's a proprietary API.
Rémy
>
> Regards,
> Violeta
> [1] https://github.com/violetagg/tomcat/commits/ws-suspend-resume
>