Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#9711 closed Bugs (fixed)

future continuation called twice

Reported by: anonymous Owned by: viboes
Milestone: Boost 1.56.0 Component: thread
Version: Boost 1.55.0 Severity: Problem
Keywords: futures Cc: ikispal@…

Description

I know it is still experimental, but I found a trivial bug in future::then().

If used with the deferred launch policy the continuation is called twice:

The first time it is called in promise::set_XXX() or future::then() as it should be, but it is called again in the get() method of the future returned by the then() method.

You can reproduce the problem by running the attached code.

Attachments (1)

boost_future.cpp (808 bytes ) - added by ikispal@… 9 years ago.

Download all attachments as: .zip

Change History (15)

by ikispal@…, 9 years ago

Attachment: boost_future.cpp added

comment:1 by ikispal@…, 9 years ago

Cc: ikispal@… added

comment:2 by ikispal@…, 9 years ago

The second time the continuation is called an empty (invalid) future is passed to it. (I suppose the guts of the future was stolen by move() during the previous call.)

comment:3 by ikispal@…, 9 years ago

What is even worse: if the future returned by then() is not void, then:

  • the first call of its get() will call the continuation a second time (as described above) and will return with the result of this erroneous second invocation!
  • the second call of its get() will return with an empty (default constructed?) value!

comment:4 by viboes, 9 years ago

Owner: changed from Anthony Williams to viboes
Status: newassigned

Thanks for catching this issue. I have not tested enough deferred futures with continuations.

comment:5 by viboes, 9 years ago

After reading more carefully the ticket and the example code, the single call must be on the call to

futr2.get();

as the then has been called with a deferred policy, and not on the promise set_value().

comment:6 by viboes, 9 years ago

Component: threadsthread
Milestone: To Be DeterminedBoost 1.56.0

Just commenting the call to execute in future_deferred_continuation_shared_state should fix the issue.

    virtual void launch_continuation(boost::unique_lock<boost::mutex>& ) {
      //execute(lk);
    }

comment:7 by anonymous, 9 years ago

If you want to start the continuation in the get() of futr2 that would mean that this will not start the continuation at all:

auto futr1 = promise1.get_future();

auto futr2 = futr1.then({});

promise1.set_value();

Is this intended? (Please note that the only get() call here is inside the continuation itself.) What about this:

auto futr1 = promise1.get_future();

auto futrLast = futr1.then({}).then({}).then({});

promise1.set_value();

Do I always have to keep track which future is at the end of the line and call its get()? My main use-case would be to not block at all, but this way I am forced to call get() and thus block in it.

in reply to:  7 comment:8 by viboes, 9 years ago

Replying to anonymous:

If you want to start the continuation in the get() of futr2 that would mean that this will not start the continuation at all:

auto futr1 = promise1.get_future();

auto futr2 = futr1.then({});

promise1.set_value();

Here you create a continuation that is asynchronously called when the futr1 is ready and so it will be called independently of whether you get the value of futr2 or not. You example used deferred

auto futr2 = futr.then(boost::launch::deferred, ...);

so that the continuation should be called only when you call futr2.get().

Is this intended?

This is how I understand it. Maybe you can disagree.

(Please note that the only get() call here is inside the continuation itself.) What about this:

auto futr1 = promise1.get_future();

auto futrLast = futr1.then({}).then({}).then({});

promise1.set_value();

Do I always have to keep track which future is at the end of the line and call its get()?

The last one is stored in futrLast. What i sthe problem?

My main use-case would be to not block at all, but this way I am forced to call get() and thus block in it.

Then you must use launch::async.

Last edited 9 years ago by viboes (previous) (diff)

comment:10 by viboes, 9 years ago

Resolution: fixed
Status: assignedclosed

comment:11 by anton.matosov@…, 8 years ago

Commit d2a65d242751cece17e28b7cc6d7bfb395e900e8 completely disables functionality of the .then(launch::deferred, ...) continuation...

I will open a separate ticket with the test code to reproduce the issue.

comment:12 by viboes, 8 years ago

I have added test for deferred then and every thing is OK https://github.com/boostorg/thread/commit/3e28ea806c412c697f134fdaacbccc9b64213426

comment:13 by anton.matosov@…, 8 years ago

Ok. I see now that I was using it differently and possibly wrong way. In the old implementation 'deferred' continuation was called synchronously from the thread that sets satisfies the future. And I am relying on this functionality. However deferred means that it should be called when feature value is retrieved, which now looks correct, but it is not that obvious when you start with future continuations as it is not described anywhere.

Is there any way to achieve the functionality of the sync call from the 'satisfier' side?

Note: See TracTickets for help on using tickets.