lucabriguglia / Weapsy

ASP.NET Core CMS
http://www.weapsy.com
Other
10 stars 2 forks source link

Uneeded Tasks #25

Closed anjmao closed 7 years ago

anjmao commented 7 years ago

Hi, just looked at Weapsy code and found some interesting parts like in SiteController.cs

        public async Task<IActionResult> Update(SiteAdminModel model)
        {
            var command = _mapper.Map<UpdateSiteDetails>(model);
            command.SiteId = SiteId;
            await Task.Run(() => _commandSender.Send<UpdateSiteDetails, Site>(command));
            return new NoContentResult();
        }

I hope you know that by running Task.Run you are scheduling execution of _commandSender.Send on thread pool, so in this case you loosing real asynchrony and it is recommended to remove async await and Taks.Run at all (it will run faster and require less resources) or better Add SendAsync implementation to commandSender so you dont need to use Task.Run

lucabriguglia commented 7 years ago

Thanks, you are absolutely right. That is legacy code, I wrote it at the beginning of the development when I was doing some experiments. Please look at other controllers that are already using the SendSync method already implemented in CommandSender. I'll fix asap this and other controllers. I'll close this issue once everything is fixed.

lucabriguglia commented 7 years ago

All controllers have been updated. I made the modified actions sync for the time being. I'll make them async again once the related async command handlers have been implemented. Thanks again :-)