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