[wix-devs] OnExecutePackageComplete without OnExecutePackageBegin

[Hoover, Jacob]

Putting that in is bad, but if we HAVE to do it...  I would envision an ExitEarlyWithSuccess macro. The real issue is going to be not jumping to LExit where all the cleanup is guaranteed to happen. 

-----Original Message-----
From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On Behalf Of Heath Stewart
Sent: Thursday, March 17, 2016 12:11 PM
To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
Subject: Re: [wix-devs] OnExecutePackageComplete without OnExecutePackageBegin

So how do people - Rob and Bob in particular - feel about an early return (i.e. "return S_OK") in this case? Since this may, in fact, set a precedent I'd like to know what's acceptable and I can code up a fix.

Thanks,

Heath Stewart
Visual Studio, Microsoft
http://blogs.msdn.com/heaths


-----Original Message-----
From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On Behalf Of Heath Stewart
Sent: Wednesday, March 16, 2016 1:45 PM
To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
Subject: Re: [wix-devs] OnExecutePackageComplete without OnExecutePackageBegin

OnCache* is fine. I was using that as an example of what is, IMO, coded correctly. But when package execution begins it checks the cache result and bails before telling the BA package execution started (which is actually in each engine's execute functions), but the general LExit block calls OnExecutePackageComplete. That's all in the code.

>From Rob, I'll file a bug and get a PR out, but proposing a true early termination via return.

Here's the gist of the code:

Execute*Package(...) {
  if (pPackage->hrCacheResult) {
    Log();
    ExitFunction1(hr = S_OK); // propose to just return S_OK;
  }
  ba->OnPackageExecuteBegin(...);
  // other stuff, all which often uses Exit*() macros
LExit:
  hr = ExecutePackageComplete(...); // calls ba->OnPackageExecuteComplete
  return hr;
}

It makes sense we bail from execute() if caching failed (no package on which to execute, in most cases anyway) but execute() was already scheduled. The runtime couldn't have known (practically) that caching would fail, so the extra check makes sense. But calling the ExitFunction macro ends up calling OnExecutePackageComplete, so we either return early or maybe skip to another label like LExit2 after LExit, in which case any future general cleanup could be done. I don't see any precedent so curious what others think.

Heath Stewart
Visual Studio, Microsoft
https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fblogs.msdn.com%2fheaths&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cf1621d6cadb1448b3ad308d34ddbcd82%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=JBWCYu5g%2blw0tqTjz6qZDqQdDeTh3j21if%2bQ95AT8XQ%3d

-----Original Message-----
From: wix-devs [mailto:wix-devs-bounces at lists.wixtoolset.org] On Behalf Of Sean Hall
Sent: Wednesday, March 16, 2016 10:10 AM
To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
Subject: Re: [wix-devs] OnExecutePackageComplete without OnExecutePackageBegin

I think every Begin callback should always have a corresponding Complete callback, even if an error occurred.  A Complete callback should never be sent unless the corresponding Begin callback was sent, if Burn is doing this then I think that's a bug.

I'd have to look at code for the OnCacheBegin/OnCacheComplete question, if the whole cache phase is skipped I don't think they should be sent.

On Wed, Mar 16, 2016 at 12:03 PM, Heath Stewart <Heath.Stewart at microsoft.com
> wrote:

> We ran into a scenario where a caching error prevented 
> OnExecutePackageBegin from executing (basically, in apply.cpp, the 
> check for the caching HRESULT happens just before calling 
> OnExecutePackageBegin on the BA) but OnExecutePackageComplete was 
> called (it's the first line after the LExit label for each 
> package-specific execute function in apply.cpp. We can work around 
> this (just keep track of what packages were started and check that set 
> in the completion callback), but begs the question whether in burn we 
> should call a Complete callback without a corresponding Begin 
> callback. It's not common to see that sort of behavior (thinking about async APIs in various frameworks I've used).
>
> What is more consistent with typical async behavior - and allows a BA 
> to clean anything specific to the downloading/caching of a package 
> still - is that ApplyCache does call OnCacheBegin invariably (i.e. no 
> exits before said call) and OnCacheComplete in the LExit (again with 
> no early exits) so they are always a pair.
>
> So should this maybe changed for wix4?
>
> Heath Stewart
> Visual Studio, Microsoft
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fblogs.
> msdn.com%2fheaths&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cebe5c
> 84c7e9141ecfa8308d34dbdd44d%7c72f988bf86f141af91ab2d7cd011db47%7c1&sda
> ta=E%2bPWtOXEyuBU3%2bx7%2fZbPH50OTH1hwEOMy1WfFptWFek%3d
>
> ____________________________________________________________________
> WiX Toolset Developer Mailing List provided by FireGiant 
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.fi
> regiant.com%2f&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cebe5c84c
> 7e9141ecfa8308d34dbdd44d%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=
> AYTU4NnSFIqWFLIZvQqVPQPcma7lhgwNr6unMJBA0XI%3d
>
____________________________________________________________________
WiX Toolset Developer Mailing List provided by FireGiant https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.firegiant.com%2f&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cebe5c84c7e9141ecfa8308d34dbdd44d%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=AYTU4NnSFIqWFLIZvQqVPQPcma7lhgwNr6unMJBA0XI%3d
____________________________________________________________________
WiX Toolset Developer Mailing List provided by FireGiant https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.firegiant.com%2f&data=01%7c01%7cHeath.Stewart%40microsoft.com%7cf1621d6cadb1448b3ad308d34ddbcd82%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=znjVw8QcKUThgGCvUdXkqfvUAO4fL3RNk4iZ6DGzbV0%3d
____________________________________________________________________
WiX Toolset Developer Mailing List provided by FireGiant http://www.firegiant.com/