[BUG] The boundary condition of the abstract class LeapArray in the sliding window under the sentinel-core package is unreasonable
- Dominant language
- Java
- Stars
- 23.1k
- Forks
- 8.1k
- PR merge metrics
- No merged PRs in 30d
Description
# [疑似缺陷]sentinel-core包下滑动窗口抽象类LeapArray边界条件判断不合理
## 【1】疑似缺陷1(BUG-1)
原始代码如下,LeapArray类第210行方法,当sampleCount为1时,取当前时间,当滑动窗口上只存在一个符合当前周期的窗口对象时,还会获取到返回结果,正确的结果应该是null才对?
The original code is as follows, the method on line 210 of the LeapArray class, when the sampleCount is 1, take the current time, when there is only one window object that matches the current period on the sliding window, it will also get the return result, the correct result should be null, right?
```
public WindowWrap getPreviousWindow(long timeMillis) {
if (timeMillis < 0) {
return null;
}
int idx = calculateTimeIdx(timeMillis - windowLengthInMs);
timeMillis = timeMillis - windowLengthInMs;
WindowWrap wrap = array.get(idx);
if (wrap == null || isWindowDeprecated(wrap)) {
return null;
}
if (wrap.windowStart() + windowLengthInMs < (timeMillis)) {
return null;
}
return wrap;
}
```
针对边界条件的判断应该改为如下:
The judgment on boundary conditions should be changed to read as follows:
```
public WindowWrap getPreviousWindow(long timeMillis) {
if (timeMillis < 0) {
return null;
}
int idx = calculateTimeIdx(timeMillis - windowLengthInMs);
timeMillis = timeMillis - windowLengthInMs;
WindowWrap wrap = array.get(idx);
if (wrap == null || isWindowDeprecated(wrap)) {
return null;
}
// if (wrap.windowStart() + windowLengthInMs < (timeMillis)) {
// return null;
// }
// [疑似缺陷]上述时间窗口的判定应该改成 wrap.windowStart()<=timeMillis bucket = array.get(idx);
if (bucket == null || !bucket.isTimeInWindow(timeMillis)) {
return null;
}
return bucket.value();
}
```
是否应该增加窗口过期判断?如下:
Should I add window expiration judgment?
```
public T getWindowValue(long timeMillis) {
if (timeMillis < 0) {
return null;
}
int idx = calculateTimeIdx(timeMillis);
WindowWrap bucket = array.get(idx);
if (bucket == null || isWindowDeprecated(bucket) || !bucket.isTimeInWindow(timeMillis)) {
return null;
}
return bucket.value();
}
```
## 【3】疑似缺陷3(BUG-3)
原始代码如下,LeapArray类第284行方法,是否缺少对入参时间的边界条件判断?还有对窗口‘valid’的判断除了过期外,是否还需要增加’超前‘窗口的判断,理论上超前的也应该不能算‘valid’的。
The original code is as follows, is the method on line 284 of the LeapArray class lacking the boundary condition judgment of the input parameter time? and the judgment of the window 'valid' in addition to expiration, whether it is necessary to add the judgment of the 'ahead' window, and theoretically the advanced should not be considered 'valid'.
```
public List> list(long validTime) {
int size = array.length();
List> result = new ArrayList>(size);
for (int i = 0; i < size; i++) {
WindowWrap windowWrap = array.get(i);
if (windowWrap == null || isWindowDeprecated(validTime, windowWrap)) {
continue;
}
result.add(windowWrap);
}
return result;
}
```
是否可以修改成如下:
Can it be modified as follows:
```
public List> list(long validTime) {
if (validTime < 0) {
return new ArrayList>();
}
int size = array.length();
List> result = new ArrayList>(size);
for (int i = 0; i < size; i++) {
WindowWrap windowWrap = array.get(i);
if (windowWrap == null || isWindowDeprecated(validTime, windowWrap) || windowWrap.windowStart() > validTime) {
continue;
}
result.add(windowWrap);
}
return result;
}
```
## 【4】疑似缺陷4(BUG-4)
原始代码如下,LeapArray类第329行方法,同疑似缺陷3,是否需要增加对‘超前’窗口的判断
The original code is as follows, the LeapArray class line 329 method, the same as BUG-3, whether it is necessary to add a judgment on the 'ahead' window
```
public List values(long timeMillis) {
if (timeMillis < 0) {
return new ArrayList();
}
int size = array.length();
List result = new ArrayList(size);
for (int i = 0; i < size; i++) {
WindowWrap windowWrap = array.get(i);
if (windowWrap == null || isWindowDeprecated(timeMillis, windowWrap)) {
continue;
}
result.add(windowWrap.value());
}
return result;
}
```
是否应该改成如下:
Should it be changed to the following:
```
public List values(long timeMillis) {
if (timeMillis < 0) {
return new ArrayList();
}
int size = array.length();
List result = new ArrayList(size);
for (int i = 0; i < size; i++) {
WindowWrap windowWrap = array.get(i);
if (windowWrap == null || isWindowDeprecated(timeMillis, windowWrap) || windowWrap.windowStart() > timeMillis) {
continue;
}
result.add(windowWrap.value());
}
return result;
}
```
## 【5】疑似缺陷5(BUG-5)
原始代码如下,LeapArray类第353行方法,该方法返回的窗口对象不一定是入参时间对应的超前一个的窗口对象,应该增加入参时间增加一个windowLength后是否落在窗口内的判断,才能正真判定返回的窗口是入参时间对应的超前一个的窗口
The original code is as follows, the LeapArray class line 353 method, the window object returned by this method is not necessarily the window object corresponding to the parameter time, and the parameter time should be increased to determine whether it falls in the window after adding a windowLength, so as to truly determine that the returned window is the window corresponding to the parameter time
```
WindowWrap getValidHead(long timeMillis) {
// Calculate index for expected head time.
int idx = calculateTimeIdx(timeMillis + windowLengthInMs);
WindowWrap wrap = array.get(idx);
if (wrap == null || isWindowDeprecated(wrap)) {
return null;
}
return wrap;
}
```
修正代码如下:
The corrected code is as follows:
```
WindowWrap getValidHead(long timeMillis) {
// Calculate index for expected head time.
int idx = calculateTimeIdx(timeMillis + windowLengthInMs);
WindowWrap wrap = array.get(idx);
if (wrap == null || isWindowDeprecated(wrap) || !wrap.isTimeInWindow(timeMillis + windowLengthInMs)) {
return null;
}
return wrap;
}
```
## END
以上缺陷修复如果合理,是否能允许我提一手PR?
If the above bug fixes are reasonable, can I be allowed to submit a PR?
Contributor guide
Assessment
This issue has not been assessed yet.