Fix #9169 refine idle timeout and failure (#10418)

Only fail request callback if a failure has not been otherwise notified.
Slight optimisation for failing idle timeouts by avoiding double lock.
Always create a failure if failing the callback.
This commit is contained in:
Greg Wilkins 2023-08-29 20:44:40 +10:00 committed by GitHub
parent 5808a62660
commit d6a0226866
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 24 additions and 23 deletions

View File

@ -346,7 +346,6 @@ public class HttpChannelState implements HttpChannel, Components
@Override @Override
public Runnable onIdleTimeout(TimeoutException t) public Runnable onIdleTimeout(TimeoutException t)
{ {
Predicate<TimeoutException> onIdleTimeout;
try (AutoLock ignored = _lock.lock()) try (AutoLock ignored = _lock.lock())
{ {
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
@ -380,29 +379,29 @@ public class HttpChannelState implements HttpChannel, Components
if (invokeOnContentAvailable != null || invokeWriteFailure != null) if (invokeOnContentAvailable != null || invokeWriteFailure != null)
return _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure); return _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure);
// Otherwise We ask any idle timeout listeners if we should call onFailure or not // otherwise, if there is an idle timeout listener, we ask if if we should call onFailure or not
onIdleTimeout = _onIdleTimeout; Predicate<TimeoutException> onIdleTimeout = _onIdleTimeout;
}
else
{
onIdleTimeout = null;
}
}
// Ask any listener what to do
if (onIdleTimeout != null) if (onIdleTimeout != null)
{ {
Runnable onIdle = () -> return _serializedInvoker.offer(() ->
{ {
if (onIdleTimeout.test(t)) if (onIdleTimeout.test(t))
{ {
// If the idle timeout listener(s) return true, then we call onFailure and any task it returns. // If the idle timeout listener(s) return true, then we call onFailure and run any task it returns.
Runnable task = onFailure(t); Runnable task = onFailure(t);
if (task != null) if (task != null)
task.run(); task.run();
} }
}; });
return _serializedInvoker.offer(onIdle); }
// otherwise, if there is no failure listener, then we can fail the callback directly without a double lock
if (_onFailure == null && _request != null)
{
_failure = Content.Chunk.from(t, true);
return () -> _request._callback.failed(t);
}
}
} }
// otherwise treat as a failure // otherwise treat as a failure
@ -476,7 +475,7 @@ public class HttpChannelState implements HttpChannel, Components
} }
// If the application has not been otherwise informed of the failure // If the application has not been otherwise informed of the failure
if (invokeOnContentAvailable == null && invokeWriteFailure == null) if (invokeOnContentAvailable == null && invokeWriteFailure == null && onFailure == null)
{ {
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("failing callback in {}", this, x); LOG.debug("failing callback in {}", this, x);

View File

@ -1177,6 +1177,7 @@ public class HttpChannelTest
request.addFailureListener(t -> error.set(null)); request.addFailureListener(t -> error.set(null));
request.addFailureListener(t -> error.compareAndSet(null, t)); request.addFailureListener(t -> error.compareAndSet(null, t));
request.addFailureListener(t -> error.compareAndSet(null, new Throwable("WRONG"))); request.addFailureListener(t -> error.compareAndSet(null, new Throwable("WRONG")));
request.addFailureListener(callback::failed);
return true; return true;
} }
}; };

View File

@ -385,6 +385,7 @@ public class ServerTest
request.addFailureListener(t -> request.addFailureListener(t ->
{ {
assertThat(ContextHandler.getCurrentContext(), sameInstance(_context.getContext())); assertThat(ContextHandler.getCurrentContext(), sameInstance(_context.getContext()));
callback.failed(t);
latch.countDown(); latch.countDown();
}); });
return true; return true;