Replace synchronized blocks with Locks

[jansupol]

No description provided.

[jbescos]

I am not sure that I understand this. Previously handler.handleTimeout(this); was invoked within a lock, and now it does not. This timeouthandler suggests that can be re-set because it is not final and contains volatile:
private volatile TimeoutHandler timeoutHandler = DEFAULT_TIMEOUT_HANDLER;

Are we sure about that missing lock?.

[jansupol]

@jbescos There is UNLOCK before the handleTimeout. handleTimeout calls writeLock.lock() and it is not possible to do with readLock locked.

[jansupol]

I understand now.

synchronized (stateLock) {
                    if (state == SUSPENDED) {
                        handler.handleTimeout(this);
                    }
}

guarded state, not handleTimeout. User defined handleTimeout can do for instance (AsyncTimoutResource in servlet-3-async integration test) :

        asyncResponse.setTimeoutHandler(new TimeoutHandler() {

            @Override
            public void handleTimeout(final AsyncResponse asyncResponse) {
                final boolean timeout2 = asyncResponse.setTimeout(millis, TimeUnit.MILLISECONDS);
                asyncResponse.setTimeoutHandler(new TimeoutHandler() {

                    @Override
                    public void handleTimeout(final AsyncResponse asyncResponse) {
                        asyncResponse.resume("timeout1=" + timeout1 + "_timeout2=" + timeout2 + "_handled");
                        asyncResponse.cancel();
                    }
                });
            }
        });

And asyncResponse.resume() has its own locks.

[jansupol]

Previously there was just one lock, whereas with ReentrantReadWriteLock it can be differenciate with readLock and writeLock, having less waiting for an unlock with multiple threads being in readLocks when there is no writeLock

[jansupol]

while previously there was one write lock, now the thread would do readLock.lock(); writeLock.lock() but that's not possible even on a single thread to upgrade the lock from read to write, first an unlock needs to happen.

[jansupol]

The user-defined TimeoutHandler cannot possibly directly access guarded fields state and cancelled to need it in a guarded block, can it? Every access to these fields is possible only through methods that are all guarded separately.

[jbescos]

Just I wanted to stress that, if for example the TimeoutHandler implementation could come from third party code, and for example contains something stupid like increasing a variable, or put something in a HashMap, or things like that, it could not work as expected, because it can be invoked by more than 1 thread at the same time.

If this case cannot happen, then it is fine as you have it.

[jansupol]

It definitely CAN happen. But should Jersey guard the customer code? I'd say not, but you are right that it would be a behavior change. Perhaps there should be another extra lock for TimoutHandler execution?

[jbescos]

To me it looks more correct to not invoke the TimeoutHandler in a lock. If the user needs it, he can synchronize it. But if we change this I think it is better to have a mention in this issue, or in a new issue, to know about this change.

[jansupol]

The TimoutHandler can be invoked simultaneously only when

• the timeout is scheduled multiple times for the same AsyncResponse, but it is already guarded by a lock in JerseyRequestTimeoutHandler
• the timeout handler timeouts at the same time on different AsyncResponses, so the lock would need to be static which is different from synchronize, which synchronizes over the instance.
• the customer would invoke the TimoutHandler method on their own, but in this case, the Lock would not be on the customer side.

In that case, I guess it is safe not to have the lock