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/04/19 10:43:50 UTC
Re: Read events suspend/resume logic in websocket impl to achieve backpressure
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 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