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. > Overall, I like the approach and would support apply a patch along these > lines once the timing issues are resolved. Thanks for the review, Violeta > <snip/> > > >>> This approach would mean some internal API changes but that is fine for > > 9.0.x and I don't see a problem with 8.5.x either. Whether this is > > back-ported to 8.0.x and 7.0.x is TBD. It also opens up the possibility of > > being able to suspend/resume other protocols but I haven't thought a great > > deal about how that might work. > >> > >> I need this functionality only for 9.0.x and 8.5.x. > > Then let's not back-port this any earlier than 8.5.x. > > Mark > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org >
