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

Compatible with CompleteFuture asynchronous programming #1323

Closed
wants to merge 36 commits into from

Conversation

dajitui
Copy link
Contributor

@dajitui dajitui commented Apr 5, 2023

Motivation:

As shown in the following link, the Sofarpc part of the code is written based on an older version of jdk, which is not compatible with the usage of jdk8 CompleteFuture. In order to make the framework more compatible with asynchronous programming, I have modified the corresponding ResponseFuture content

#945 (comment)

Modification:

My idea is that ResponseFuture directly inherits CompleteFuture, as it already inherits Future and CompletionStage, and there is no need to implement classes to override these methods in the future

Result:

Fixes #.

@sofastack-bot sofastack-bot bot added cla:yes CLA is ok size/S labels Apr 5, 2023
@OrezzerO OrezzerO self-requested a review April 5, 2023 15:28
@@ -28,5 +28,4 @@ jobs:
- name: Test with Maven
run: ./mvnw package -Pci-test
- name: Codecov
uses: codecov/codecov-action@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woc,I heard about it for the first time

@@ -316,4 +316,4 @@ protected void setDoneTime() {
public long getElapsedTime() {
return doneTime - genTime;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know why unit testing is unstable? Sometimes through unit testing, occasionally with exceptions

Copy link
Contributor

Choose a reason for hiding this comment

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

@EvenLjj We need to make our unit test stable. It is disgusting to try it again and again :( . How about add a task to solve it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, some single tests require reconstruction, we will kick of.

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ad29f00) 72.01% compared to head (09a51f7) 72.06%.
Report is 30 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1323      +/-   ##
============================================
+ Coverage     72.01%   72.06%   +0.04%     
- Complexity      783      784       +1     
============================================
  Files           416      416              
  Lines         17659    17663       +4     
  Branches       2752     2752              
============================================
+ Hits          12718    12729      +11     
+ Misses         3538     3529       -9     
- Partials       1403     1405       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sofastack-bot sofastack-bot bot removed the size/S label Apr 6, 2023
@nobodyiam nobodyiam closed this May 5, 2023
@nobodyiam nobodyiam reopened this May 5, 2023
@sofastack-cla sofastack-cla bot added size/L and removed size/M labels May 6, 2023
@dajitui
Copy link
Contributor Author

dajitui commented May 16, 2023

@OrezzerO @EvenLjj This merger request is a bit long, do you have time to help review it and merge branch?

@EvenLjj
Copy link
Collaborator

EvenLjj commented May 16, 2023

@OrezzerO @EvenLjj This merger request is a bit long, do you have time to help review it and merge branch?

OK, but this PR will also affect the internal version, and we will review in the next iteration and merge after passing the internal regression test.

@dajitui
Copy link
Contributor Author

dajitui commented May 16, 2023

@OrezzerO @EvenLjj This merger request is a bit long, do you have time to help review it and merge branch?

OK, but this PR will also affect the internal version, and we will review in the next iteration and merge after passing the internal regression test.

OK, i get it

Copy link
Contributor

@OrezzerO OrezzerO left a comment

Choose a reason for hiding this comment

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

 @Test
    public void thenApply() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        MyFuture myFuture = new MyFuture(1000);
        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        myFuture.thenAccept(consumer);

        myFuture.setSuccess("done");

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

    @Test
    public void thenApply_correctVersion() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        CountDownLatch countDownLatch = new CountDownLatch(1);
        CompletableFuture<String> future = CompletableFuture.supplyAsync(() -> {
            // 模拟耗时操作
            try {
                countDownLatch.await();
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
            return "Hello, World!";
        });

        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        future.thenAccept(consumer);

        countDownLatch.countDown();

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

I write two test case . The first one is aim to test AbstractResponseFuture, and the second one shows how CompletableFuture should work. Please make sure the first case can pass.

@dajitui
Copy link
Contributor Author

dajitui commented May 18, 2023

 @Test
    public void thenApply() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        MyFuture myFuture = new MyFuture(1000);
        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        myFuture.thenAccept(consumer);

        myFuture.setSuccess("done");

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

    @Test
    public void thenApply_correctVersion() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        CountDownLatch countDownLatch = new CountDownLatch(1);
        CompletableFuture<String> future = CompletableFuture.supplyAsync(() -> {
            // 模拟耗时操作
            try {
                countDownLatch.await();
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
            return "Hello, World!";
        });

        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        future.thenAccept(consumer);

        countDownLatch.countDown();

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

I write two test case . The first one is aim to test AbstractResponseFuture, and the second one shows how CompletableFuture should work. Please make sure the first case can pass.

@Test
    public void thenApply() throws InterruptedException {
        AtomicBoolean applied = new AtomicBoolean(false);
        MyFuture myFuture = new MyFuture(1000);
        Consumer<? super String> consumer = (String str) -> {
            System.out.println(str);
            applied.compareAndSet(false, true);
        };
        myFuture.thenAccept(consumer);

        myFuture.setSuccess("done");

        boolean isDone = myFuture.complete("done");

        Thread.sleep(1000);

        Assert.assertTrue(applied.get());

    }

I found that future does not perform an operation, it needs to call complete method to modify the state @OrezzerO

@OrezzerO
Copy link
Contributor

OrezzerO commented May 24, 2023

Yes, you are right. So there is a compatibility issues here. We need to make sure that when future task is complete, then it trigger 'then apply' task. complete method is provided by CompletableFuture , it will not invoked by sofarpc code. We need to invoke method such as complete in sofarpc some where.

@dajitui dajitui requested a review from OrezzerO May 24, 2023 08:30
@OrezzerO
Copy link
Contributor

I think this pr is ok. @EvenLjj Please check compatibility with internal version .

@stale
Copy link

stale bot commented Jul 28, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jul 28, 2023
@dajitui
Copy link
Contributor Author

dajitui commented Jul 28, 2023

@EvenLjj Buddy, this pr is a bit long and will be marked as expired. Evaluate whether the merger will affect the existing code, thanks

@stale stale bot removed the wontfix This will not be worked on label Jul 28, 2023
@stale
Copy link

stale bot commented Sep 27, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Sep 27, 2023
@EvenLjj EvenLjj closed this Oct 3, 2023
@EvenLjj EvenLjj reopened this Oct 3, 2023
@stale stale bot closed this Oct 10, 2023
@OrezzerO OrezzerO reopened this Nov 12, 2023
@stale stale bot removed the wontfix This will not be worked on label Nov 12, 2023
Copy link

stale bot commented Jan 11, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jan 11, 2024
@stale stale bot closed this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes CLA is ok size/L wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants