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-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org >
