2017-03-20 15:41 GMT+01:00 Violeta Georgieva <miles...@gmail.com>: > Hi, > > 2017-02-27 16:50 GMT+02:00 Mark Thomas <ma...@apache.org>: > > > > On 27/02/17 11:55, Violeta Georgieva wrote: > > > > <snip/> > > > > >> A new patch is available based on the provided comments. > > >> Can you please review it. > > > > > > Any feedback for the latest changes > > > > Sorry for the delay. > > > > On a minor/style point, I'd prefer SUSPENDED rather than READ_SUSPENDED > > since the socket won't be eligible for read or write. > > Ok > > > Thinking some more about that, could that cause problems? Does the patch > > need to ensure write operations aren't attempted? Non-blocking writes > > should be OK but a blocking write would be problematic. > > I was thinking something > around org.apache.tomcat.websocket.WsRemoteEndpointBasic.send* methods. > If the reading is suspended then these methods will do nothing and log > error. > What do you think? > > > I think there is still a timing / concurrency issue around resume(). > > Consider the following sequence: > > - incoming message is being processed in WsFrameServer > > - suspend() is called on another thread > > - while loop ends and onDataAvailable returns > > - resume() is called on another thread > > - then WsHttpUpgradeHandler checks if the thread is suspended > > > > The problem is between onDataAvailable returning and > > WsHttpUpgradeHandler checking if the thread is suspended. If resume() is > > called and processed during that admittedly narrow gap, the socket will > > end up in the Poller twice which - from past experience - will cause > > problems. > > I fixed that. The PR https://github.com/apache/tomcat/pull/42 is updated. > > A long time ago there were two extensions proposed for the "comet" API (still in the wiki somehow: https://wiki.apache.org/tomcat/WhatIsComet ). As part of them was suspend/resume which do exactly what you propose: suspend read events for a while. While the Servlet API additions also looks very similar, it decided to be less flexible and in particular there's no suspend/resume (conceptually with the Servlet API, you'd have to unset the listeners which isn't allowed).
So, having looked at the history, it's hard for me to really complain about this being added. Rémy
