Let me go, this is a bug!

Hello, I am why.

I went to Dubbo unknowingly when I was hanging out on Git two days ago.

Looking at the data in the last month, the community is still very active:

Then I took a look at the latest issue, and everyone asked questions actively.

I saw such an issue and found it interesting:

https://github.com/apache/dubbo/issues/8055

So I wrote this article to share with you this bug and the story behind the bug.

Don't worry, even if you don't understand Dubbo at all, it won't affect your understanding of this bug.

Let me talk about it first, the Dubbo code mentioned below, where there is no special description, are the codes on the Master branch I pulled from git

What a bug?

Let me first describe what this BUG is like.

In fact, it was written by the author of this issue: There is a problem with the Filter sorting process in the Dubbo framework. Even after writing the rules according to the framework requirements, the final Filter chain generated is not what we want.

So friends who don’t understand Dubbo at all may encounter the first question: What is Filter?

In fact, it's just a filter, and it's conceptually the same as a filter in a web service. And Dubbo has a lot of Filters, which together form a Filter call chain.

Quoting a call link diagram on the official website, I framed it in the place of Filter:

You can see that Filter is a very core component of the Dubbo framework, and many, many functions are extended from Filter.

It doesn't matter if you don't understand it yet, as long as you know that there is such a Filter call chain, the Filters on the chain perform their duties and do their own things.

Okay, so now the demand comes:

I now require that the execution order of the Filter on the chain is under my control, that is, when I define the Filter, you have to leave a place for me to set its priority.

It sounds like a simple requirement, right?

I will leave you a hole directly, let you enter the order parameter without entering a default value, and then sort the order according to the Order when assembling the Filter chain.

It’s not that I’m bragging, I can finish writing in ten minutes, with three minutes of fishing in the middle.

However, just such a demand caused a BUG.

What is the specific phenomenon?

I will pull down the project here, based on the official test cases, change from Pakistan to Pakistan, and show you what the manifestation of this BUG is.

There is such a note in Dubbo:

org.apache.dubbo.common.extension.Activate

The Order here is used for sorting. To demonstrate briefly, you see that I now have 5 Filters:

The sorting rule is that the smaller the Order, the first to execute, then the execution order of this Filter chain should be like this:

Filter4 -> Filter3 -> Filter2 -> Filter1 -> Filter5

Make a test case, let's verify:

In line with expectations, there is nothing wrong with it.

In addition, the official test case about Filter is here. If you are interested, you can see the source code after you pull it down:

org.apache.dubbo.common.extension.support.ActivateComparatorTest#testActivateComparator

Whether it is an official case or a case written by myself, the most critical sorting function is implemented by this line of code:

Collections.sort(filters, ActivateComparator.COMPARATOR)

The most important thing in this line of code is ActivateComparator.COMPARATOR .

This thing is the source of BUG, don't panic, let's talk about it later.

So why is it said that it has a BUG?

The previous demonstration shows that under normal circumstances, it is in line with expectations.

But look at the Activate , there are two things like this:

Before and after, the meaning is to specify that Filter A is executed before or after Filter B.

But it was marked with @Deprecated annotation, and the field description also noted:

Deprecated since 2.7.0

It was abolished after 2.7.0.

Then it's a bit interesting, why was it abolished?

Let’s take a look at an example. It’s the test case just now.

I just changed it slightly like this:

@Activate(before = "_2")
public class Filter5 implements Filter0{
}

The change point is to configure on Filter5:

@Activate(before = "_2")

The meaning is that Filter5 is executed before "_2".

What is "_2"?

It's just a mapping of Filter2:

Then the problem is coming. As a normal programmer, after confidently making this change to Filter5, his inner thought must be to put such a Filter chain:

Filter4 -> Filter3 -> Filter2 -> Filter1 -> Filter5

Modified as follows: (Filter5 is executed before Filter2):

Filter4 -> Filter3 -> Filter5 -> Filter2 -> Filter1

So what is the actual situation?

Let's run:

What's the matter? Is this not the execution chain I expected?

Yes, this is the manifestation of BUG.

What's the matter?

What is going on?

Let me analyze a wave for you.

As I said in the last section, the problem lies in the sorting algorithm.

org.apache.dubbo.common.extension.support.ActivateComparator

Come, let's take a look:

First, the place labeled ① is to encapsulate before, after, and order, and then provide several comparison methods. You know that there are these things in the ActivateInfo entity, which will be used in the following code.

Then talk about the place labeled ②.

Don't look at this place is quite long, but the logic is actually very simple. If any one of the two filters currently compared is configured with before and after, it will enter the logic of the part labeled ②.

Then the logic inside is like this:

The specific logic is not elaborated, and I will give you an intuitive demonstration later.

The last label is ③ This place is a bit interesting, so I will say a few more words.

Being able to go to the place labeled ③ means that the two filters currently being compared are not configured with the two attributes of after and before.

Just compare Order directly.

This place also does a special treatment for the case where Order is equal:

o1.getSimpleName().compareTo(o2.getSimpleName()) > 0 ? 1 : -1

If the Orders are equal, compare the class names.

The reason for this is to ensure the stability of sorting.

For example, for these two Filters, no Order is specified:

Then if we remove this judgment:

The code becomes like this:

if (a1.order > a2.order) {
    return 1 
} else {
    return -1 
    
}

To simplify things like this:

return a1.order > a2.order ? 1:-1

Then this piece of code, the whole will become like this:

Take a closer look like this: Hey, it seems that it can be optimized. Line 78 and line 80 are the same, so line 78 can be omitted.

Okay, after such a transformation.

Congratulations, got an old version of the code:

On the left is the code of the previous version, and on the right is the code of the current Master branch:

There must be a reason why the change occurred.

Take a look at the submission record:

This submission points to submission number 7778:

https://github.com/apache/dubbo/pull/7778

And this submission points to issue number 7757:

https://github.com/apache/dubbo/issues/7757

And this issue is also mentioned in the issue number 8055 mentioned earlier:

This issue is mainly two pictures.

The first picture is like this:

In the absence of any custom Filter, only the official original Filter, the constructed Filter chain, ExecuteLimitFilter is before MonitorFilter.

The second picture is like this:

After adding a series of custom Filters (without specifying Order), ExecuteLimitFilter is ranked after MonitorFilter.

As for the impact of these two filters in the front row and back row, they are not related to the text and will not be expanded. If you are interested, you can check the corresponding links.

In short, only this kind of judgment logic is unstable:

return a1.order > a2.order ? 1:-1

Let's demonstrate with an example:

The left is the test case, the right is the sorting rule, and the following is the output result.

As you can see from the output results, the final Filter chain depends on the order in which the list is added.

This is what the 7757 issue says:

The traversal order of the list will affect the sorting order.

Therefore, there will be such a submission:

Okay, now we change the sort order back, and run the same test case again, and it's stable:

A sharp-eyed friend may also have discovered a problem.

There is another submission from this place:

• The first judgment: return o1.getSimpleName().compareTo(o2.getSimpleName())
• The second judgment: return o1.getSimpleName().compareTo(o2.getSimpleName())> 0? 1: -1;

What are you doing?

The first judgment also neglects the situation where the package name is different but the class name is the same:

• com.why.a.whyFilter
• com.why.b.whyFilter

At this time, o1.getSimpleName().compareTo(o2.getSimpleName()) returns 0.

What happens when 0 is returned?

Just swallow a Filter, believe it or not?

For example, your collection is HashSet or TreeMap.

This is a coincidence, Dubbo uses TreeMap.

Let's demonstrate with a test case.

If the first judgment is adopted, finally there is only one Filter in the TreeMap:

If the second judgment is used, there will be two Filters in the TreeMap at the end:

The details, the devil is in the details.

Oh, it's really hard to guard against.

Okay, I'll finish talking about the comparator, but you have found no. I haven't told you what is the BUG of unstable sorting process. I just introduced another BUG for you.

Don't panic, it's not that I haven't figured out how to describe it to you.

This process is actually more complicated and involves the Timsort sorting method. This method will have to be explained in another article.

So, I changed my mind to show you the process of comparison and the reasons behind this process.

Timsort is doing a ghost, and you are welcome to explore it yourself.

What is the process?

I added this output statement at the entrance of the comparison method:

The five filters are like this:

The test case looks like this:

@Test
public void whyTest(){
    List<Class> filters = new ArrayList<>();
    filters.add(Filter4.class);
    filters.add(Filter3.class);
    filters.add(Filter2.class);
    filters.add(Filter1.class);
    filters.add(Filter5.class);
    Collections.sort(filters, ActivateComparator.COMPARATOR);
    StringBuilder builder = new StringBuilder();
    for (int i = 0; i < filters.size(); i++) {
        builder.append(filters.get(i).getSimpleName()).append("->");
    }
    System.out.println(builder.toString());
}

The output log looks like this:

Did you find the problem?

First of all, I carefully controlled the order in which the list was added:

In this way, the first three comparisons can build such a Filter chain:

Filter4->Filter3->Filter2->Filter1->

Then, Filter5 first compares with Filter1 after it comes in, and finds that its Order is 0 greater than Filter1's -1, so the comparison ends, and this Filter chain is obtained:

Filter4->Filter3->Filter2->Filter1->Filter5->

During the whole process, Filter5 and Filter2 did not undergo any comparison operation, let alone the before label in Filter5:

But when I change the order in which the list is added:

Hey, that's correct, you said God is not magical?

Isn't it magical?

why?

Let's take a look at the principle of Timsort.

Tracing the source

In fact, I have written a question here:

Who and when did the after/before mechanism be introduced?

Because of this mechanism, I personally think that the starting point is quite good. There is one more configuration place and the choice is left to the user.

But in actual use, it is prone to confusion.

So I took a look at the submission record:

This annotation was first written by Liang Fei (one of the pioneers of the Dubbo project), and there was no before and after at the beginning of the design, but there was a match and mismatch.

Then at 1:54 a.m. one day after writing this comment, a method-level match was submitted:

These three methods are even more complicated to use than before/after.

So at 12:34 after waking up, Liang Fei deleted these three configurations:

Two months later, on May 8, 2012, after and before configurations were added:

Then it stayed in the Dubbo source code until 6 years later, on August 7, 2018, with a comment that is not recommended:

And mentioned this issue:

https://github.com/apache/dubbo/issues/2180

It says: After and before are not used in the Dubbo source code, and the sorting is problematic.

So these two methods, after version 2.7.0, were marked as not recommended, announcing the death of the method.

I don't know why Liang Fei introduced these two methods in 2012. I also wanted to find some clues from his code submission records, but unfortunately there were none.

However, there is another idea:

After Liang Fei introduced these two methods back then, did the comparator he wrote take this situation into consideration?

So I immediately looked at the code submission record of the comparator:

org.apache.dubbo.common.extension.support.ActivateComparator

And copied his code, and ran it with the same test case:

Unfortunately, there is the same problem.

Perhaps these two methods should not be introduced back then.

From the main road to the Jane, learning Spring's Order, there is only one Order:

Then I suddenly thought of another framework: SofaRPC.

SofaRPC and Dubbo and HSF are inextricably linked to love and hatred, so I took a look at the corresponding place of SofaRPC:

com.alipay.sofa.rpc.ext.Extension

For sorting, only order is reserved.

So the code of the comparator is very simple:

com.alipay.sofa.rpc.common.struct.OrderedComparator

In addition, by the way, I compared the code of the comparator written by Liang Fei and the latest comparator. The function is exactly the same, but the code is quite different:

I have to say that after several refactorings, the newest comparator is much more readable.

I tracked the submission record of this class, and watched the evolution of this class step by step, which is actually a good example of code refactoring.

If you are interested, go through it yourself.

Okay, that's it, finish work.