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. 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 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. Overall, I like the approach and would support apply a patch along these lines once the timing issues are resolved. <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
