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