webmvc-adapter: improve set entry in request to avoid ErrorEntryFreeException

[cdfive]

Describe what this PR does / why we need it

The AbstractSentinelInterceptor of sentinel-spring-webmvc-adapter may enter twice under some circumstances, such no view page exists

Does this pull request fix one issue?

Relate to #1531, #1482

Describe how you did it

Refator setEntryInRequest method, if attribute key already exists in request, we don't call SphU.entry to create Entry, then Entry get from request can exit normally in afterCompletion method.

Describe how to verify it

Add a tmp test Controller in sentinel-demo-spring-webmvc to verify if it works.

@Controller
@RequestMapping("/")
public class TmpTestController {

    @RequestMapping("/view_not_exists")
    public String view_not_exists() {
        return "/view_not_exists.html";
    }
}

Special notes for reviews

The tmp test Controller is used for debug and test, so it has not been committed.

[codecov-commenter]

Codecov Report

Merging #1533 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1533      +/-   ##
============================================
+ Coverage     44.05%   44.08%   +0.02%     
- Complexity     1735     1737       +2     
============================================
  Files           376      376              
  Lines         10696    10696              
  Branches       1427     1427              
============================================
+ Hits           4712     4715       +3     
+ Misses         5417     5415       -2     
+ Partials        567      566       -1     

Impacted Files	Coverage Δ	Complexity Δ
...ter/spring/webmvc/AbstractSentinelInterceptor.java	70.00% <100.00%> (ø)	13.00 <0.00> (ø)
...l/cluster/server/connection/ConnectionManager.java	74.00% <0.00%> (-2.00%)	10.00% <0.00%> (-1.00%)
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java	100.00% <0.00%> (+4.16%)	8.00% <0.00%> (+1.00%)
...s/block/flow/controller/RateLimiterController.java	89.65% <0.00%> (+10.34%)	8.00% <0.00%> (+1.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a97226...95f50e3. Read the comment docs.

[CLAassistant]

All committers have signed the CLA.

[jasonjoo2010]

Talk to the design it's a little dirty to drop it if request was forwarded again.

Before further discussion let's figure out one small problem: Does servlet 2.x still can be used currently? Is there a limitation for implementation of adapter?

If the answer is yes we may be forced to introduce a simple detecting logic.

Since servlet 3.0 new methods were introduced into ServletRequest including getDispatherType() which can make things more elegant. DispatherType has several values like REQEUST / FORWARD / INCLUDE and others.
In the scenario of issue the type is set to REQUEST initially and set to FORWARD later after fallback. The REQUEST type will wrap them all outside. like an onion:

So whether we do it elegantly or just eat it with no sense? Let make a decision, @sczyh30 and @cdfive ?

[sczyh30]

Before further discussion let's figure out one small problem: Does servlet 2.x still can be used currently? Is there a limitation for implementation of adapter?

We might need to keep support for Servlet 2.x. It could be better if we could identify the dispatcher type and handle it for Servlet 3.x+.

[jasonjoo2010]

Before further discussion let's figure out one small problem: Does servlet 2.x still can be used currently? Is there a limitation for implementation of adapter?

We might need to keep support for Servlet 2.x. It could be better if we could identify the dispatcher type and handle it for Servlet 3.x+.

Then reflection may could be introduced and leave fallback only for servlet2.x. In fallback we also can use object/arraylist in request to make it more elegant.

Any other idea?

[jasonjoo2010]

FYI

To get the servlet major version number there is another way which should be examined carefully under 2.x is request.getServletContext().getMajorVersion()

[sczyh30]

Before further discussion let's figure out one small problem: Does servlet 2.x still can be used currently? Is there a limitation for implementation of adapter?

We might need to keep support for Servlet 2.x. It could be better if we could identify the dispatcher type and handle it for Servlet 3.x+.

Then reflection may could be introduced and leave fallback only for servlet2.x. In fallback we also can use object/arraylist in request to make it more elegant.

Okay for me :)

@cdfive Any other suggestions?

[cdfive]

目前咱们的sentinel-web-servlet和sentinel-spring-webmvc-adapter都以provided方式依赖了javax.servlet-api，版本是3.1.0，实际使用中如果项目中使用了SpringBoot，比如像sentinel-dashboard，使用了spring-boot-starter-web版本，依赖如下：

spring-boot-starter-web(2.0.5RELEASE)
  └─spring-boot-starter-tomcat(2.0.5RELEASE)
      └─tomcat-embed-core(8.5.34)

其中tomcat-embed-core提供了javax.servlet-api相同的相关包和类。
启动dashboard工程，debug查看
request.getServletContext().getMajorVersion()和request.getServletContext().getMinVersion()分别是3和1。
实现类在org.apache.catalina.core.ApplicationContext类里面的getMajorVersion()和getMinVersion()，用的常量3和1。

dashboard依赖的2.0.5RELEASE版本不算很新，但可能也是主流，我在公司不同项目里经常看到这个版本，或者较新的2.2.x。

Since servlet 3.0 new methods were introduced into ServletRequest including getDispatherType() which can make things more elegant
We might need to keep support for Servlet 2.x. It could be better if we could identify the dispatcher type and handle it for Servlet 3.x+.

区分2.x和3.x清楚了，但没有明白如何分别支持它们，如果大部分是3.x，是否有必要支持2.x呢

Talk to the design it's a little dirty to drop it if request was forwarded again.

这个有点困惑，forward不是咱们的adaptor里做的呀，好像是springmvc内部处理的。
目前本PR修改前后逻辑是：
之前：先创建Entry，然后判断HttpServletRequest是否有已有属性，如果有打印警告，没有则将Entry绑定到Request。
之后：判断HttpServletRequest是否有已有属性，如果有打印警告，没有才创建Entry，并绑定到Request。

之前的问题是虽然打印了警告，但Entry仍然创建了，因为springmvc可能处理后forward了又进了拦截器，导致Entry退出不配对。
如果抛开DispatherType的问题，感觉之后这个逻辑更合理一些。

PS：@jasonjoo2010 的图表和描述非常详细，涨知识👍

[jasonjoo2010]

目前咱们的sentinel-web-servlet和sentinel-spring-webmvc-adapter都以provided方式依赖了javax.servlet-api，版本是3.1.0，实际使用中如果项目中使用了SpringBoot，比如像sentinel-dashboard，使用了spring-boot-starter-web版本，依赖如下：
...

关于3.x处理

这个很简单，当不是request类型的时候，可以考虑在key加一个前缀（例如把枚举字符串表示加上如FORWARD-）

关于是否需要兼容2.x

这个取决于是否能在普通的spring项目使用，可以的话，是存在这个可能性[1]

关于2.x处理

2.x因为无法区分dispather type，比起做简单的丢弃处理，其实也可以考虑在key碰撞的情况下，测试其类型是否是Entry，如果是的话，追加上去的时候换成Stack/List即可实现进出栈。普通的时候不用可以减少临时对象的产生，且这个逻辑可以与做了key在3.x中的区分相关逻辑共存，无需特殊处理。

Reference:

[1] Difference between versions of tomcat http://tomcat.apache.org/whichversion.html

[cdfive]

@jasonjoo2010 @sczyh30
这是spring-webmvc的adaptor，肯定是使用了spring或者springboot的项目才会用到。
目前是打印warn前就创建Entry，只是想优化这个逻辑。
因为可能对于90%请求，都只是进一次HandlerInterceptor，对于少部分请求，像issue里那样，
但adaptor不应该出现Error，打印警告即可。
咱们能否简化下思路，目前这个PR先完善该逻辑，确实丢弃有些简单，但能解决issue里的问题，并且有警告打印能提醒。
比起之前不丢弃但导致Error，对于用户来说体验可能会好一些。

对于该拦截器，使用上是可以配置的：

registry.addInterceptor(new SentinelWebInterceptor(config)).addPathPatterns("/**").excludePathPatterns(xxx);

像issue里遇到，有可能他排除相关path即可。但由于没有设置，因为adaptor的问题导致Error。

对于servlet 2.x, 3.x，给key加上forward前缀以及key碰撞用Stack/List处理，
有需要的话单独开一个PR来处理，这样可行否？
此外，我觉得这种forward的情况是否需要再创建Entry有待讨论。

[jasonjoo2010]

@jasonjoo2010 @sczyh30
这是spring-webmvc的adaptor，肯定是使用了spring或者springboot的项目才会用到。
目前是打印warn前就创建Entry，只是想优化这个逻辑。
因为可能对于90%请求，都只是进一次HandlerInterceptor，对于少部分请求，像issue里那样，
但adaptor不应该出现Error，打印警告即可。
咱们能否简化下思路，目前这个PR先完善该逻辑，确实丢弃有些简单，但能解决issue里的问题，并且有警告打印能提醒。
比起之前不丢弃但导致Error，对于用户来说体验可能会好一些。

对于该拦截器，使用上是可以配置的：

registry.addInterceptor(new SentinelWebInterceptor(config)).addPathPatterns("/**").excludePathPatterns(xxx);

像issue里遇到，有可能他排除相关path即可。但由于没有设置，因为adaptor的问题导致Error。

对于servlet 2.x, 3.x，给key加上forward前缀以及key碰撞用Stack/List处理，
有需要的话单独开一个PR来处理，这样可行否？
此外，我觉得这种forward的情况是否需要再创建Entry有待讨论。

目前的处理，问题在于实际上会发生多次的entry和exit，会在不正确的exit中退出，从逻辑上说的确是存在问题的，哪怕简单粗暴地使用stack也能更正确些

[jasonjoo2010]

According to the elaboration in comments entry will exit at wrong place. Patch based on reference counting will be another PR.

LGTM