Add future default methods #180

MrIvanPlays · 2022-05-03T17:57:35Z

As discussed here on the Discord server.

To-do:

Resolve remaining comments.
?..

Basically, this will be the 2nd and last breaking change we will do in v2 - removing subscribers - what this is adding is default CompletableFuture implementations.

lokka30 · 2022-05-03T23:21:34Z

Clarification (to my understanding of the PR here, please let me know if this is wrong)

This PR is a step in implementing a breaking change to Treasury where CompletableFuture methods become the primary ones, and Subscription methods are supplied as default methods which internally just wrap CompletableFutures (opposite of the current code).
This PR will achieve:
- Easier for people to use CF methods in Treasury as they no longer have to wrap their subscription methods using Subscriber#asFuture
This PR does not make the breaking change where CFs become primary, rather than Subscriptions.
- This PR will only affect economy consumer plugins - economy provider plugins will need to use subscriptions or some type of wrapper dealing with them, until we make the breaking change.

Chat log in case anyone is interested for context

(The chat log has been slightly modified to make it clearer)

@lokka30

Ivan - a thought came across my mind and I'd like to hear your opinion:

Adding overloads on the subscriber methods for CompletableFuture conversions, instead of having to manually wrap it with Subscriber#asFuture.

These are just variants of the existing methods that return a CompletableFuture instead of using a subscriber parameter 😃 e.g.

// subscriber version
void retrievePlayerAccount(
    @NotNull UUID accountId,
    @NotNull EconomySubscriber<PlayerAccount> subscription
)

// CF version
@NotNull
default CompletableFuture<PlayerAccount> retrievePlayerAccount(
    @NotNull UUID accountId
)

// both methods are present in the same file.
// nothing is removed. this is just a short hand that
// makes using CFs less tedious in Treasury.

in theory this will be cleaner to use than wrapping the subscriber method with Subscriber#asFuture.

People whinge about having to write concurrent code, so I am trying to find ways to make it easier for them to write. this is vital if we aim to have any adoption in the community.

@MrIvanPlays

Yes it will be cleaner I agree on this @lokka30
They will be default methods
like
default Future retrieve(...) {
Future future = ...
retrieve(...)
return future
Currently on phone
Tommorrow ill see what I can do
We should have used futures from the start but your concerns were 'its hard for newbies'

@lokka30

Subscriptions are a useful feature, though Treasury should have always used CompletableFutures as the standard methods and provide defaulted subscription methods which just use CFs under the hood (opposite to what we have currently).
Glad to hear you like the idea 👍

Regarding the Subscription/CF thing: I still firmly vote that we should make a breaking change in Treasury v2 where CFs are the standard instead of Subscriptions. It will help us in the long term. This is the last and only breaking change I vote to make to Treasury, assuming we do not repeat the current mistakes in any new APIs.

Another regret I have is releasing Treasury on SpigotMC with a release version (v1) rather than a beta version (v0) - a beta version would have otherwise allowed us to trial APIs for real world use without having to be very scared of the rare breaking change.

I think we have ironed out almost everything with the economy API as is; it seems the only issues remaining are the subscriber/CF scenario, and the lack of documentation (which I will work on in my break).

@MrIvanPlays

Documentation is something which can be done at any state

Unfortunately, I have a concern which if we do such breaking changes every major release of treasury then devs will view treasury as a 'can break always' API and push them away

@lokka30

I would dread using a library/plugin that breaks their API often - this is something I am currently dealing with for one of my own plugins (which try offer compat for another plugin which constantly breaks their API).

I think we have only made one breaking change so far, which was to remove the API versioning system. I think this CF/Sub scenario is important enough that it deserves a breaking change soon. It will make it much easier for economy providers to be written since they can use subscriptions at their own option. This would be breaking change number 2 and almost certainly the last breaking change we will need to make to the economy API... I sincerely hope.

For existing economy providers, they can either shift their usage of subscriptions to CFs, or they can do this:

override defaults for subscription methods to not use the default, and rather, use the subscription code they've alreay made
make the CF methods redirect to the subscription methods they've made
Using CFs will only make provider code cleaner so it's up to them really.

MrNemo64

Why are we using HashSets as collection of completable futures when CompletableFuture does not override hashCode ?
I have commented some places that I've seen that exceptions may be thrown that doesn't seem to be handled. I may have mist some

https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/concurrent/CompletableFuture.java

MrNemo64 · 2022-05-04T13:30:37Z

api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java

+            return FutureHelper.mjf(account -> account.isMember(playerId),
+                    Account::getIdentifier,
+                    futures
+            );


FutureHelper#mjf uses CompletableFuture#join() which

throws an (unchecked) exception if completed exceptionally

but this excepcion is not handled in the mjf method or this method, nor is it specifyed in the documentation that it's not handled

MrNemo64 · 2022-05-04T13:37:19Z

api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java

+            return FutureHelper.mjf(
+                    account -> account
+                            .hasPermission(playerId, permissions)
+                            .thenApply(triState -> triState == TriState.TRUE),
+                    Account::getIdentifier,
+                    accounts
+            );


FutureHelper#mjf uses CompletableFuture#join() which

throws an (unchecked) exception if completed exceptionally

but this excepcion is not handled in the mjf method or this method, nor is it specifyed in the documentation that it's not handled

MrNemo64 · 2022-05-04T13:40:57Z

...bukkit/plugin/src/main/java/me/lokka30/treasury/plugin/bukkit/hooks/papi/economy/BalTop.java

-                BigDecimal balance = EconomySubscriber
-                        .<BigDecimal>asFuture(s -> account.retrieveBalance(currency, s))
-                        .join();
+                BigDecimal balance = account.retrieveBalance(currency).join();


join() may throw an unchecked exception thats not handled

MrNemo64 · 2022-05-04T13:41:47Z

.../plugin/src/main/java/me/lokka30/treasury/plugin/bukkit/hooks/papi/economy/BalanceCache.java

-                BigDecimal balance = EconomySubscriber
-                        .<BigDecimal>asFuture(s -> account.retrieveBalance(currency, s))
-                        .join();
+                BigDecimal balance = account.retrieveBalance(currency).join();


join() may throw an unchecked exception thats not handled

MrIvanPlays · 2022-05-04T13:50:52Z

Why are we using HashSets as collection of completable futures when CompletableFuture does not override hashCode ?

I have commented some places that I've seen that exceptions may be thrown that doesn't seem to be handled. I may have mist some

https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/concurrent/CompletableFuture.java

CompletableFuture does nothing about the result but return it after it receives it. So it doesn't matter which collection type is used.
Re: exceptions: if an exception is thrown, it will be captured by the CompletableFuture and will call the exception handler, specified by CompletableFuture#exceptionally. It usually isn't our responsibility to handle them, but it looks like the mjf method will need a modification due to the nature of the code that uses it.

Jikoo

Not a lot to add beyond MrNemo64's concerns. Nice catch of the unsafe operations in the PAPI hook.

I do still somewhat worry about inexperienced developers' implementations (it's easy to make mistakes, as we ourselves displayed in the PAPI hook!) as expressed in #161 (comment) and other referenced places, but this really is what I wanted from Treasury in the first place.

api/src/main/java/me/lokka30/treasury/api/common/misc/FutureHelper.java

MrIvanPlays · 2022-05-04T16:33:20Z

I think I have another idea instead of futures and that is adding a way of combining subscriptions. Something like

retrieveAccount(uuid, Subscription.combine(account -> account.retrieveBalance(...)));

In half an hour I can come up with a better example (currently phone), but what I have in mind is probably the best as it is not breaking change and is much cleaner approach than asFuture and raw subscription.

MrIvanPlays · 2022-05-04T17:26:34Z

@Jikoo @lokka30

    static <A, B> EconomySubscriber<A> thenCall(
            BiConsumer<A, EconomySubscriber<B>> methodCall, EconomySubscriber<B> subscription
    ) {
        return new EconomySubscriber<A>() {
            @Override
            public void succeed(@NotNull final A a) {
                methodCall.accept(a, subscription);
            }

            @Override
            public void fail(@NotNull final EconomyException exception) {
                subscription.fail(exception);
            }
        };
    }

example usage:

        provider.retrievePlayerAccount(
                uuid,
                EconomySubscriber.thenCall(
                        (account, subscription) -> account.retrieveBalance(
                                provider.getPrimaryCurrency(),
                                subscription
                        ),
                        new EconomySubscriber<BigDecimal>() {

                            @Override
                            public void succeed(@NotNull BigDecimal balance) {
                                // ...
                            }

                            @Override
                            public void fail(@NotNull EconomyException exception) {
                                // ...
                            }
                        }
                )
        );

what do you think?

MrIvanPlays · 2022-05-05T06:54:25Z

(it's easy to make mistakes, as we ourselves displayed in the PAPI hook!)

.join is not used mistakenly in the papi hook - as you know the papi hook's cache loads are being run asynchronously, so calling .join has no damage at all. Yes it will throw the exception if the future failed, just like any other thing you do if it fails it throws an exception.

lokka30

I dismiss my previous approval as I would like MrNemo and Jikoo's requested changes to be satisfied. Otherwise, I'm all ready to merge this in.

Ivan - if you would like me to work on anything in this branch, please let me know.

(I couldn't find where the 'dismiss review' feature was in GH, so I just posted this new review instead.)

MrIvanPlays · 2022-05-05T07:31:12Z

I dismiss my previous approval as I would like MrNemo and Jikoo's requested changes to be satisfied. Otherwise, I'm all ready to merge this in.

Ivan - if you would like me to work on anything in this branch, please let me know.

(I couldn't find where the 'dismiss review' feature was in GH, so I just posted this new review instead.)

what about doing the idea i gave instead of continuing this?

MrNemo64 · 2022-05-05T09:39:58Z

(it's easy to make mistakes, as we ourselves displayed in the PAPI hook!)

.join is not used mistakenly in the papi hook - as you know the papi hook's cache loads are being run asynchronously, so calling .join has no damage at all. Yes it will throw the exception if the future failed, just like any other thing you do if it fails it throws an exception.

Since we are the ones that write the papi hook, we are the ones in charge of handling the exception, arent we? If on the cache the future fails, no one is handling the exception right now

@Jikoo @lokka30

    static <A, B> EconomySubscriber<A> thenCall(
            BiConsumer<A, EconomySubscriber<B>> methodCall, EconomySubscriber<B> subscription
    ) {
        return new EconomySubscriber<A>() {
            @Override
            public void succeed(@NotNull final A a) {
                methodCall.accept(a, subscription);
            }

            @Override
            public void fail(@NotNull final EconomyException exception) {
                subscription.fail(exception);
            }
        };
    }

example usage:

        provider.retrievePlayerAccount(
                uuid,
                EconomySubscriber.thenCall(
                        (account, subscription) -> account.retrieveBalance(
                                provider.getPrimaryCurrency(),
                                subscription
                        ),
                        new EconomySubscriber<BigDecimal>() {

                            @Override
                            public void succeed(@NotNull BigDecimal balance) {
                                // ...
                            }

                            @Override
                            public void fail(@NotNull EconomyException exception) {
                                // ...
                            }
                        }
                )
        );

what do you think?

I have to give some thought to this

lokka30 · 2022-05-05T12:17:31Z

Subscription chaining is a separate feature. I don't see why we can't add subscription chaining in, in addition to this CF defaults change.

api/src/main/java/me/lokka30/treasury/api/common/misc/FutureHelper.java

* Make FutureHelper#mapJoinFilter future-friendly * The method implementation now performs operations in an entirely non-blocking fashion. * Renamed to #mapJoinFilter per other suggestions. * Changed the filter parameter to be non-null: all usages were non-null anyway. * Change usage of FutureHelper.mapJoinFilter accordingly * Replace thenComposeAsync with thenCompose * Use pre-sized ArrayList rather than HashSet * Use consistent variable name

lokka30 · 2022-09-27T03:05:04Z

I've added a to-do list to the original post of this pull request so we can keep track of what changes need to be made regarding the concurrency changes in Treasury v2.

Since work on this project seems to be in sparse bursts, I hope that it'll help us stay on track, rather than trying to reassemble what needs to be done each time.

Please edit this to-do list. [link to top]

MrIvanPlays · 2022-10-30T15:20:17Z

Will be addressed in a different pr to branch 2.0.0

add future default methods

be492f0

MrIvanPlays added 2 commits May 4, 2022 09:00

finish

684d5a4

switch to using future methods directly in baltop stuff

bca5f6d

MrIvanPlays changed the title ~~[WIP] Add future default methods~~ Add future default methods May 4, 2022

MrIvanPlays marked this pull request as ready for review May 4, 2022 06:10

MrIvanPlays requested review from lokka30 and Jikoo and removed request for lokka30 May 4, 2022 06:11

lokka30 approved these changes May 4, 2022 •

edited

Loading

View reviewed changes

This comment was marked as resolved.

Sign in to view

MrNemo64 reviewed May 4, 2022

View reviewed changes

Jikoo reviewed May 4, 2022

View reviewed changes

api/src/main/java/me/lokka30/treasury/api/common/misc/FutureHelper.java Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

lokka30 requested review from lokka30 and removed request for lokka30 May 5, 2022 07:04

lokka30 requested changes May 5, 2022

View reviewed changes

lokka30 mentioned this pull request Jul 1, 2022

Suggestion: Treasury Integration TownyAdvanced/Towny#5752

Closed

This comment was marked as resolved.

Sign in to view

lokka30 mentioned this pull request Jul 17, 2022

Treasury Economy Integration Team-Inceptus/Novaconomy#2

Closed

A248 reviewed Aug 26, 2022

View reviewed changes

api/src/main/java/me/lokka30/treasury/api/common/misc/FutureHelper.java Outdated Show resolved Hide resolved

This was referenced Aug 26, 2022

Make FutureHelper#mapJoinFilter future-friendly #196

Merged

Fix some thread safety and exception-related problems #195

Merged

lokka30 marked this pull request as draft August 31, 2022 00:49

MrIvanPlays mentioned this pull request Oct 12, 2022

improve papi economy hook #210

Merged

MrIvanPlays closed this Oct 30, 2022

MrIvanPlays deleted the convert/futures branch October 30, 2022 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add future default methods #180

Add future default methods #180

MrIvanPlays commented May 3, 2022 •

edited by lokka30

Loading

lokka30 commented May 3, 2022 •

edited

Loading

@lokka30

@MrIvanPlays

@lokka30

@MrIvanPlays

@lokka30

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

MrNemo64 left a comment

MrNemo64 May 4, 2022

MrNemo64 May 4, 2022

MrNemo64 May 4, 2022

MrNemo64 May 4, 2022

MrIvanPlays commented May 4, 2022

Jikoo left a comment

MrIvanPlays commented May 4, 2022

MrIvanPlays commented May 4, 2022

This comment was marked as resolved.

This comment was marked as resolved.

MrIvanPlays commented May 5, 2022

lokka30 left a comment •

edited

Loading

MrIvanPlays commented May 5, 2022

MrNemo64 commented May 5, 2022 •

edited

Loading

lokka30 commented May 5, 2022

This comment was marked as resolved.

This comment was marked as resolved.

lokka30 commented Sep 27, 2022

MrIvanPlays commented Oct 30, 2022

Add future default methods #180

Add future default methods #180

Conversation

MrIvanPlays commented May 3, 2022 • edited by lokka30 Loading

lokka30 commented May 3, 2022 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

MrNemo64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrIvanPlays commented May 4, 2022

Jikoo left a comment

Choose a reason for hiding this comment

MrIvanPlays commented May 4, 2022

MrIvanPlays commented May 4, 2022

This comment was marked as resolved.

This comment was marked as resolved.

MrIvanPlays commented May 5, 2022

lokka30 left a comment • edited Loading

Choose a reason for hiding this comment

MrIvanPlays commented May 5, 2022

MrNemo64 commented May 5, 2022 • edited Loading

lokka30 commented May 5, 2022

This comment was marked as resolved.

This comment was marked as resolved.

lokka30 commented Sep 27, 2022

MrIvanPlays commented Oct 30, 2022

MrIvanPlays commented May 3, 2022 •

edited by lokka30

Loading

lokka30 commented May 3, 2022 •

edited

Loading

lokka30 left a comment •

edited

Loading

MrNemo64 commented May 5, 2022 •

edited

Loading