Hi, 2017-03-20 16:41 GMT+02: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. > > > Overall, I like the approach and would support apply a patch along these > > lines once the timing issues are resolved.
Any feedback about the new changes? 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 > > >
