Skip to content

Future #1286

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 26, 2016
Merged

Future #1286

merged 2 commits into from
Oct 26, 2016

Conversation

@stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Oct 26, 2016

Two patches improving futures.

  • simplify AbstractListenableFuture.executionList initialization
  • Allow null executor in ListenableFuture

@stepancheg
simplify AbstractListenableFuture.executionList initialization
6f00431 
Just use CAS loop instead of synchronized and two boolean variables.
slandelle
slandelle reviewed Oct 26, 2016
View reviewed changes
client/src/main/java/org/asynchttpclient/future/AbstractListenableFuture.java Outdated
executionList = localExecutionList;
executionListInitialized = true;
}
for (;;) {
Copy link
Contributor

@slandelle slandelle Oct 26, 2016

Choose a reason for hiding this comment

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

Are you sure we need this?

Copy link
Contributor Author

@stepancheg stepancheg Oct 26, 2016

Choose a reason for hiding this comment

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

Not sure what you meant. Loop is not needed indeed. I've updated the PR.

Copy link
Contributor

@slandelle slandelle Oct 26, 2016

Choose a reason for hiding this comment

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

That was exactly my thought. :)

@slandelle
Copy link
Contributor

slandelle commented Oct 26, 2016

Nice catch, thanks! I do have a question though.

@stepancheg
Allow null executor in ListenableFuture
8f6d2a2 
It allows execution of future callback in completion thread, and
makes `ListenableFuture` closer to `CompletableFuture` (which allows
`null` executor).

It also saves a little memory in `toCompletableFuture` implementation.
@slandelle slandelle merged commit 8048931 into AsyncHttpClient:master Oct 26, 2016
@stepancheg
Copy link
Contributor Author

stepancheg commented Oct 26, 2016

Wait!

I made a mistake in addListener:

    @Override
    public ListenableFuture<V> addListener(Runnable listener, Executor exec) {
        if (executionList == null) {
            ExecutionList.executeListener(listener, exec); // <-- HERE
            return this;
        }

        executionList().add(listener, exec);
        return this;
    }

Revert it please.

@stepancheg
Copy link
Contributor Author

stepancheg commented Oct 26, 2016

Going to submit alternative patch.

@stepancheg stepancheg mentioned this pull request Oct 26, 2016
Merged
@stepancheg
Copy link
Contributor Author

stepancheg commented Oct 26, 2016

Revert for the first part is #1288, better patch for the first part is #1287.

@slandelle
Copy link
Contributor

slandelle commented Oct 26, 2016

@stepancheg I deleted the commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slandelle slandelle slandelle left review comments

Assignees

No one assigned

Labels

Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stepancheg @slandelle