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