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

Add something similar to DecodeResult.NOT_FOUND to ServiceInvocationHandler #45

Closed
trustin opened this issue Nov 30, 2015 · 4 comments
Closed
Assignees
Milestone

Comments

@trustin
Copy link
Member

trustin commented Nov 30, 2015

A ServiceCodec can tell the session layer that the request doesn't actually belong to the enclosing Service so that the other Service takes over, by returning DecodeResult.NOT_FOUND.

It will be nice if ServiceInvocationHandler has something similar. For example, a user could bind an HttpFileService and a TomcatSerivce under the same path prefix. If HttpFileService fails to find the file, TomcatService could handle it, making it possible to serve the static resources using Armeria. (TomcatService could also handle the 404 response from Tomcat specially so that the next service takes over.)

/cc @delegacy

@trustin trustin added this to the 0.6.0.Final milestone Nov 30, 2015
@delegacy
Copy link
Contributor

delegacy commented Dec 1, 2015

Agreed. However, I'm not sure how to generalize a trigger to take over requests for the next service.

@trustin
Copy link
Member Author

trustin commented Dec 1, 2015

This issue will require two following changes:

  • Make DecodeResult.NOT_FOUND not fail immediately. Let HttpServerHandler try again until it tries all matching services.
  • Define a mechanism for ServiceInvocationHandler that does the same job with DecodeResult.NOT_FOUND.

Perhaps we could define some exception type? We have ServiceNotFoundException already, so we could use it for this purpose. It's actually the exception used when HttpServerHandler gets DecodeResult.NOT_FOUND, so it seems to make sense to me.

Another issue to consider is the case where a service wants to tell the session layer that the path does not exist and it does not want other services to take the request over. However, I'm not sure if we should consider this because usually they will just end up hitting the catch-all path mapping or making HttpServerHandler return 404.

@delegacy
Copy link
Contributor

delegacy commented Dec 1, 2015

Another issue to consider is the case where a service wants to tell the session layer that the path does not exist and it does not want other services to take the request over. However, I'm not sure if we should consider this because usually they will just end up hitting the catch-all path mapping or making HttpServerHandler return 404.

I think we need to consider the case when a user does want the last service to catch 404 and response with the custom error page. In this case, propagation of 404 should not be done.

@trustin trustin modified the milestones: 0.6.0.Final, 0.7.0.Final Dec 8, 2015
@trustin trustin self-assigned this Dec 15, 2015
@trustin
Copy link
Member Author

trustin commented Dec 15, 2015

This actually was fixed in #60 slightly differently.

@trustin trustin closed this as completed Dec 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants