Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust resolution/rejection of a promise #77

Merged
merged 1 commit into from
Jan 5, 2016

Conversation

trustin
Copy link
Member

@trustin trustin commented Dec 31, 2015

Motivation:

When a client does not receive a response or a server fails to process a
request within time limit, an invocation promise is rejected regardless
the final outcome of the invocation.

In such a case, a ServiceInvocationHandler or a remote invoker
implementation might attempt to resolve or reject the promise which is
already rejected with TimeoutException. Handling this sort of situations
is very common, and thus we should provide it as part of Armeria.

Modifications:

  • Add ServiceInvocationContext.resolvePromise() and rejectPromise() to
    provide a simple and robust way to resolve or reject a promise in a
    ServiceInvocationHandler
  • Use resolvePromise() and rejectPromise() wherever possible.
  • Miscellaneous:
    • Fix a bug where WriteTimeoutException is raised rather than
      ResponseTimeoutException
    • Fix a bug where HttpSessionHandler.failPendingResponses() does not
      fail anything at all
    • Make TimeoutException traceless to reduce its instantiation overhead

Result:

  • Robustness
    • Fixes potential issues when attempting to resolve a rejected promise.
    • Late responses are now silently discarded to avoid log message
      flooding

@trustin
Copy link
Member Author

trustin commented Dec 31, 2015

I wonder if we should make all timeout exceptions stacktraceless to avoid the overhead. Thoughts?

@He-Pin
Copy link

He-Pin commented Dec 31, 2015

@trustin,if the stacktrace is not that useful but the message and the type mater,I think this would be great for the performance.

@trustin trustin force-pushed the robust_promise_resolution branch 5 times, most recently from 67c20c4 to c7622ab Compare December 31, 2015 18:54
@trustin
Copy link
Member Author

trustin commented Dec 31, 2015

@hepin1989 Yep. Made the change in TimeoutException already.

}

try {
if (!(promise.cause() instanceof TimeoutException)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think timeouts should still be logged to info

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe they're already logged in timeout handler? If so then it's ok, I guess it's very exceptional to even get to this code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TimeoutException has already failed the promise. This if block is to swallow the result arrived after the timeout.

Armeria does not log any exceptions raised by a Service implementation. It uses the exceptions to produce proper error responses, and that's all. To log any exceptions raised by a Service, you'll have to decorate it. e.g. using LoggingService.

Motivation:

When a client does not receive a response or a server fails to process a
request within time limit, an invocation promise is rejected regardless
the final outcome of the invocation.

In such a case, a ServiceInvocationHandler or a remote invoker
implementation might attempt to resolve or reject the promise which is
already rejected with TimeoutException. Handling this sort of situations
is very common, and thus we should provide it as part of Armeria.

Modifications:

- Add ServiceInvocationContext.resolvePromise() and rejectPromise() to
  provide a simple and robust way to resolve or reject a promise in a
  ServiceInvocationHandler
- Use resolvePromise() and rejectPromise() wherever possible.
- Miscellaneous:
  - Fix a bug where WriteTimeoutException is raised rather than
    ResponseTimeoutException
  - Fix a bug where HttpSessionHandler.failPendingResponses() does not
    fail anything at all
  - Make TimeoutException traceless to reduce its instantiation overhead

Result:

- Robustness
  - Fixes potential issues when attempting to resolve a rejected promise.
  - Late responses are now silently discarded to avoid log message
    flooding
anuraaga added a commit that referenced this pull request Jan 5, 2016
More robust resolution/rejection of a promise
@anuraaga anuraaga merged commit 1ffb8d0 into line:master Jan 5, 2016
@trustin
Copy link
Member Author

trustin commented Jan 5, 2016

@anuraaga Thanks for merging :-)

@trustin trustin deleted the robust_promise_resolution branch January 5, 2016 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants