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

PublishAsync is not truly async #16

Closed
guy-lud opened this issue Dec 14, 2015 · 18 comments
Closed

PublishAsync is not truly async #16

guy-lud opened this issue Dec 14, 2015 · 18 comments

Comments

@guy-lud
Copy link

guy-lud commented Dec 14, 2015

Hello flq,

Firstly I like to say that I think that your implementation is quite remarkable and very extendable, things that needed in today development world.

I’ve been playing with Membus trying it out and i think that the PublishAync method lack the full power of async await. By using it with an async action it not really "waiting" for the task result so by using Membus on a web server Membus actually consume worker threads.

The issue is in the SequentialPipeline at the LookAtAsync it calls the push method without waiting for it (and using Task.Run() which schedule tasks).

WDYT?

@flq
Copy link
Owner

flq commented Dec 15, 2015

Hi, thanks for your kind words and feedback. I am currently looking at the code in terms of getting it running on the dnx core framework - I'll also have a look at the place you mention, see if there is something weird about it.

@flq
Copy link
Owner

flq commented Dec 15, 2015

Okay, so I've been looking at this ... http://stackoverflow.com/questions/13489065/best-practice-to-call-configureawait-for-all-server-side-code

and from what I understand, it seems normal that async/await will hit the worker thread pool. Am I missing something?

@guy-lud
Copy link
Author

guy-lud commented Dec 16, 2015

Hi flq,
in web server for example each request get one thread, when using async / await we want to do it on IO threads which are OS managed so it won't take working threads from users.

By using Task.Run() we are "taking" threads from the threadpool thus from the users.

we want to use the Push mehod to return task and in the SequentialXXX class should call PushAsync and the user of the library to use await.

you can test it in the subscibe method use await like so :

    public class Program
{
    public static  void Main(string[] args)
    {
        IBus bus = BusSetup
            .StartWith<Conservative>()
            .Construct();

        bus.Subscribe<Fo>(async x =>
        {
            await Do(x);
        });

        bus.Publish(new Fo());
        Console.WriteLine("No good");
        Console.ReadLine();
    }

    public static Task Do(Fo fo)
    {
        return Task.Delay(10000);
    }
}

public class Fo
{

}

@frostebite
Copy link

did this get resolved?

@flq
Copy link
Owner

flq commented Jun 7, 2018

Nope, as I don’t know what the resolution is.

@frostebite
Copy link

frostebite commented Jun 7, 2018

What is the intended async flow? Of course there is the publish async but there doesn't seem to be a flow for making an async subscriber? And by merit of the way Action seems to be used. It seems like maybe there needs to be an explicit flow for Action<T, Task>.

@frostebite
Copy link

@flq looking at the source it appears async is only partially implemented? Did this not get finished or am I missunderstanding there?

It looks like 'IAsyncPublishingPipelineMember' isn't really used anywhere. And the BlockingParrallelPublish doesn't use async methods.

@frostebite
Copy link

How would you feel about collaborating to get async working? :)

@flq
Copy link
Owner

flq commented Jun 8, 2018

Indeed, async was quite an afterthought to have a possibility to await the execution of a publish Operation.

The trouble I have with this lib: I have no use case for it anymore. I found it super-helpful in building rich clients, but have done a lot of web dev in the past years, so poor MemBus has been neglected somewhat (Somebody even published a MemBus-reborn to get targetting right). However, if you have concrete ideas, maybe we can invest some time.

@frostebite
Copy link

frostebite commented Jun 8, 2018

I'd be super happy to loop you in on how we're using membus!

I think you can see below the like TL;DR of how we're using membus though. I'd be super happy to work with you on a solution as well if that's helpful!

public class MemBusTest {
    [Fact]
    public async Task TestIng() {
        string message = "A";
        var membus = BusSetup.StartWith<AsyncConfiguration>().Construct();
        membus.Subscribe(
            async (string firedMsg) => {
                await Task.Delay(10000);
                message = "B";
            }
        );

        await membus.PublishAsync("");
        Assert.Equal(message, "B");
    }
}

@frostebite
Copy link

frostebite commented Jun 8, 2018

In short the solution to my problem and the orginal posted would be for there to be a path where TaskFactory/Task.Run() isn't used. It simply sequentially calls await and follows and async flow through each subscriber.

I.E no step along the way through the membus, not the final subscribed method is a non async method and all must also return Task. Perhaps Task although that isn't within my use case :)

@frostebite
Copy link

It's also worth noting none of the other configurations are working correctly in this case. And if I understand correctly. The parrallel blocking publisher should. (Which is the one used by RichClientFrontend configuration)

@flq
Copy link
Owner

flq commented Jun 8, 2018

So I suppose we’re talking about a deviation from the current behavior and introducing a fully async pipeline?

@frostebite
Copy link

Yes indeed :)

@frostebite
Copy link

@flq that would exactly be it actually yes. Looking at it there is already a partially complete async pipeline?

@frostebite
Copy link

A further alternative would be some of the private fields could be made protected and this would make it easier to build this myself with just inherited classes and new configurations.

@flq
Copy link
Owner

flq commented Jun 10, 2018

I think it’d be cool if the solution is in MemBus, so anybody in need of an async flow can use this. We could do some kind of skype / hangout to flesh out some details and then go for it.

@flq
Copy link
Owner

flq commented Jun 17, 2018

This issue will be closed in favour of #27 , which will track progress on adding a fully async pipeline to MemBus for the Milestone release 4.2.

@flq flq closed this as completed Jun 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants