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

#1669 for discussion only #1676

Merged
merged 6 commits into from Aug 16, 2021
Merged

#1669 for discussion only #1676

merged 6 commits into from Aug 16, 2021

Conversation

ctlove0523
Copy link
Contributor

In current version,reactor netty hasn't really concept about ‘route’,we can't define route directly use code.In reactory netty,HttpRouteHandler is the route,when one request come in,we iterate HttpRouteHandler list to find one HttpRouteHandler can handle income request,if no HttpRouteHandler can handle request return http status code 404(mean no route). In the iterate process we use Predicate<? super HttpServerRequest> to match request one by one,when Predicate<? super HttpServerRequest> return true,it means there is one route for this request,so i think in reactory netty Predicate<? super HttpServerRequest> is the "route".

@violetagg violetagg added this to the 1.0.x Backlog milestone Jun 17, 2021
@violetagg violetagg linked an issue Jun 17, 2021 that may be closed by this pull request
@violetagg violetagg marked this pull request as draft June 17, 2021 11:17
@violetagg
Copy link
Member

@ctlove0523 I converted this PR to draft.

@04-30yan
Copy link

How to code my dns with other Parsing logic

@violetagg
Copy link
Member

@04-30yan May be you are mixing the topics? I'll encourage you to use Gitter for questions related to Reactor Netty.

@OlegDokuka
Copy link
Contributor

@ctlove0523 just wanna add the general comment that the logic sounds like a filter logic where you based on the incoming request filters it or not.

Though it can work for some use cases, it cannot help in scenarios where one needs to replace a handler with an equivalent one.

Copy link
Contributor

@OlegDokuka OlegDokuka left a comment

Choose a reason for hiding this comment

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

LGTM in general with some minor polishes

@OlegDokuka OlegDokuka marked this pull request as ready for review July 15, 2021 14:40
@violetagg violetagg modified the milestones: 1.0.x Backlog, 1.0.10 Jul 19, 2021
@roggenbrot
Copy link
Contributor

roggenbrot commented Jul 20, 2021

Nice work.

Only Problem I see is that I can only specify the route that should be removed by path/method.

In my use case I have to deal with unknown services that have no knowledge of each other.

Imagine the following scenario:

1.) First OSGI service is started and adds a new route with path /test
2.) Second OSGI service starts and adds a new route with path /test
3.) Second OSGI is stopped and tries to remove the route he has added on start (cleanup)

The current solution would remove both routes (Route of OSGI service "First" and "Second"), or did I miss something?

@ctlove0523
Copy link
Contributor Author

Nice work.

Only Problem I see is that I can only specify the route that should be removed by path/method.

In my use case I have to deal with unknown services that have no knowledge of each other.

Imagine the following scenario:

1.) First OSGI service is started and adds a new route with path /test
2.) Second OSGI service starts and adds a new route with path /test
3.) Second OSGI is stopped and tries to remove the route he has added on start (cleanup)

The current solution would remove both routes (Route of OSGI service "First" and "Second"), or did I miss something?

In one HttpServer you can add many same route,but if you not order same routes,request will match first added route.From your description you want a concept like route owner,owner can only remove routes which added by the owner,but HttpServer has't this concept.

@roggenbrot
Copy link
Contributor

roggenbrot commented Jul 21, 2021

In one HttpServer you can add many same route,but if you not order same routes,request will match first added route

That's correct. But adding a duplicate route will not harm the first added route. The first registered route will work, even if you register millions of routes with the same path/predicate.

Problem I see is that if you not aware of all other routes (e.g. App with Plugin Concept, OSGI ......), you can remove a existing route accidentally when you try to remove "your" route.

You will remove the first (working) route and "harm" the application since a route which was previously working suddenly does not to work, although your intention was to remove just "your" route.

Debugging such "suddenly disappearing" routes could be a nightmare .....

With wildcard predicate handlers it might be even worse:

If you register a wildcard predicate handler, that should return a nice 404 page on every request which was not matched by a previous handler, using removeIf will always remove also such a handler. There is no chance to prevent this, which makes the removeIf unusable in such scenarios.

Don't get me wrong. I really like your work and I really appreciate all the effort you put into it.

All I'm saying is that there might be uses cases where this solution might cause trouble since it's only safe to use it if you are aware of all added routes (Which might be even a problem when multiple teams work on the same project).

I don't know/ can not decide if these use cases are valid and should be supported by reactor-netty, someone else has to make this decision.

From your description you want a concept like route owner,owner can only remove routes which added by the owner,but >HttpServer has't this concept.

You are right, but maybe it should have such concept (Returning Subscription, add route via publisher .........)

@ctlove0523
Copy link
Contributor Author

@roggenbrot
Thanks,the core issue is how to remove routes.If there are three routes,call remove method once,only remove one route?There are two ways may helpful.
Not allowed to add the same route
add same route more than once may be code issue and it doesn't make much sense .

when add route return an object which can identity added route
If we can get route's identity then can remove the route we want.

@violetagg
Copy link
Member

violetagg commented Jul 27, 2021

@roggenbrot
Thanks,the core issue is how to remove routes.If there are three routes,call remove method once,only remove one route?There are two ways may helpful.
Not allowed to add the same route
add same route more than once may be code issue and it doesn't make much sense .

when add route return an object which can identity added route
If we can get route's identity then can remove the route we want.

@roggenbrot Do you think that if we add something like a route identity, it will solve your use case? I do not have information for your solution but in your case it may be OSGi service name or whatever will give you the information whether or not to remove the route.

@violetagg
Copy link
Member

violetagg commented Aug 9, 2021

@ctlove0523 @roggenbrot I'll postpone this for 1.0.11 so that we can clarify the comment #1676 (comment)

@violetagg violetagg modified the milestones: 1.0.10, 1.0.11 Aug 9, 2021
@violetagg
Copy link
Member

@ctlove0523 @roggenbrot I'm going to commit this change.

@roggenbrot We can reopen your issue if you think that #1676 (comment) may help for your use case.

@violetagg violetagg merged commit fab25e1 into reactor:main Aug 16, 2021
@ctlove0523
Copy link
Contributor Author

@ctlove0523 @roggenbrot I'm going to commit this change.

@roggenbrot We can reopen your issue if you think that #1676 (comment) may help for your use case.

Ok,if there are other requirements, i will continue to this.

@violetagg
Copy link
Member

Thank you all for the discussion and the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to remove a added route from HttpServerRoutes
5 participants