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-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
