Azure-functions-durable-extension: WaitForExternalEvent uncancellable timer fix was dropped from v2.0

[AtOMiCNebula]

Opening a new bug to track this; original bug was #816.

@ConnorMcMahon / @cgillum Sadly, it looks like my original change for #816 was completely blown away shortly after getting merged into dev, and none of it ended up getting included in the v2 release. ☹️

As best as I can tell, it looks like the fault lies with 780749d3a3fbbc648b04e71e2d17afc1efe789d4; there were tons of merge conflicts, but no work appears to have been done to actually resolve most of them, instead just ignoring them.

Trying to replay that merge locally yields the following:

You have unmerged paths.
  (fix conflicts and run "git commit")
  (use "git merge --abort" to abort the merge)

Changes to be committed:

        modified:   .github/ISSUE_TEMPLATE/bug_report.md
        new file:   .github/ISSUE_TEMPLATE/feature_request.md
        new file:   .github/ISSUE_TEMPLATE/new-release-template.md
        modified:   samples/v1/javascript/package-lock.json
        modified:   samples/v1/javascript/package.json

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)

        both modified:   samples/v1/precompiled/VSSample.csproj
        deleted by us:   src/WebJobs.Extensions.DurableTask/DurableOrchestrationContext.cs
        deleted by us:   src/WebJobs.Extensions.DurableTask/DurableOrchestrationContextBase.cs
        both modified:   src/WebJobs.Extensions.DurableTask/DurableTaskExtension.cs
        deleted by us:   src/WebJobs.Extensions.DurableTask/DurableTaskOptions.cs
        both modified:   src/WebJobs.Extensions.DurableTask/Listener/TaskOrchestrationShim.cs
        both modified:   src/WebJobs.Extensions.DurableTask/Microsoft.Azure.WebJobs.Extensions.DurableTask.xml
        both modified:   src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj
        both modified:   test/Common/DurableTaskEndToEndTests.cs
        both modified:   test/Common/TestHelpers.cs
        both modified:   test/Common/TestOrchestrations.cs
        both modified:   test/FunctionsV1/WebJobs.Extensions.DurableTask.Tests.V1.csproj

What's the right plan of action here? Apologies for not noticing this sooner, I thought you were going to attempt a v1.8.4 release prior to shipping v2.0. ☹️

[AtOMiCNebula]

I think my commit was the only one truly lost, though I'm not sure about 53176ef. All impacted commits:

• 859ccc8 Added feature request template
  
  
  
  • Merged cleanly, not lost
  
  
• 024d370 Add WaitForExternalEvent with CancellationToken overloads (#820)
  
  
  
  • LOST
  
  
• edff2fc Adding small change to accommodate the way Python worker passes JSON (#875)
  
  
  
  • Not truly lost, #876 merged this into v2 specifically
  
  
• d092757 Update issue templates
  
  
  
  • Merged cleanly, not lost
  
  
• be570ad Increment nuget package version to v1.8.3 (#827)
  
  
  
  • Not truly lost, parts were carried into v2 earlier
  
  
• 53176ef Add automatic fetching of large messages to Durable 1.0 (#824)
  
  
  
  • LOST, or heavily refactored
  
  
• 08ee0fe Revert Microsoft.Azure.Webjobs to version 2.2.0
  
  
  
  • Not lost
  
  
• b7fbc61 Updated JavaScript samples' dependencies: * Microsoft.Azure.WebJobs.Extensions.DurableTask: 1.8.0 -> 1.8.2 * durable-functions: 1.1.2 -> 1.2.2
  
  
  
  • Probably not lost
  
  

Z:\azure-functions-durable-extension>git show --no-patch 780749d
commit 780749d3a3fbbc648b04e71e2d17afc1efe789d4
Merge: 7fc6c25 859ccc8
Author: Connor McMahon <[email protected]>
Date:   Tue Aug 27 16:37:05 2019 -0700

    Merge branch 'dev' into merge-dev-into-v2

Z:\azure-functions-durable-extension>git merge-base 7fc6c25 859ccc8
f2e92b866aed1d085eed33d2a7bf50d9aecfb70d

Z:\azure-functions-durable-extension>git log --oneline f2e92b866aed1d085eed33d2a7bf50d9aecfb70d..859ccc8 --first-parent
859ccc8 Added feature request template
024d370 Add WaitForExternalEvent with CancellationToken overloads (#820)
edff2fc Adding small change to accommodate the way Python worker passes JSON (#875)
d092757 Update issue templates
be570ad Increment nuget package version to v1.8.3 (#827)
53176ef Add automatic fetching of large messages to Durable 1.0 (#824)
08ee0fe Revert Microsoft.Azure.Webjobs to version 2.2.0
b7fbc61 Updated JavaScript samples' dependencies: * Microsoft.Azure.WebJobs.Extensions.DurableTask: 1.8.0 -> 1.8.2 * durable-functions: 1.1.2 -> 1.2.2

[ConnorMcMahon]

@AtOMiCNebula, I think this was somewhat intentional for your PR.

When we merged the dev branch into v2, we dropped your changes, because we were considering make a breaking change to the API surface area instead of going with your backwards compatible approach. However, we didn't track this well on our end, and our window for this breaking change has now passed, so we should probably just port your fix to v2 now.

Sorry for the inconvenience this caused you. It should hopefully be straightforward to get this into the 2.1.0 release.

[AtOMiCNebula]

Alright. My expectation was that you'd at least get the change into v2, and consider removing the "old" APIs separately. I wasn't expecting it to disappear completely. :innocent:

I can maybe rebase my original change against the current dev branch sometime this week or next, assuming that's alright with you.

[ConnorMcMahon]

December 13th is our soft date at the moment.

[AtOMiCNebula]

@ConnorMcMahon / @cgillum Since the port of this needs to (re-)add new CancellationToken-aware APIs to IDurableOrchestrationContext, is that an acceptable breaking change? I can't layer it like was done previously with DurableOrchestrationContextBase/DurableOrchestrationContext. It's not clear to me how I'd leverage DurableContextExtensions for this, since the only type function code now sees is IDurableOrchestrationContext, and I don't think you'd want extensions to do tricky cast-and-check-style stuff inside the extension methods.

[ConnorMcMahon]

@AtOMiCNebula,

On the one hand, adding new methods to an interface is definitely a breaking change. However, considering the vast majority of "implementation" of the interface are likely just mocks, customer code should be fairly resilient to adding new methods to the interface. That being said, it is still a breaking change, which we have so far worked very hard to avoid (minus the 2.0 release obviously). Also, having to make this breaking change every time we want to add some new functionality seems indicative of a design problem.

We have already been considering re-adding in the abstract classes, as the interfaces + extension method approach has led to a lot of developer pain in writing unit tests. Maybe in 2.1, we reintroduce the abstract classes, and change the documentation back to using those instead of the interfaces. We obviously couldn't deprecate the interfaces, but we could encourage developers to use the abstract classes instead, as this is the only place we could add new core functionality.

This backtracking would be annoying for customers who just went through the effort of switching to the interface approach after we went out of preview, but it may be the only way to avoid these sorts of issues in the future.

[AtOMiCNebula]

@ConnorMcMahon / @cgillum Do you have any further thoughts on which way you think you want things to go with potential additional interface changes (of a breaking nature)?

Personally, as a consumer of your library, I like the current state of things (i.e. the IFooClient interfaces, instead of FooClientBase/FooClient). The interfaces are very clear, and the interfaces feel a bit cleaner (since the variants have all been moved to static methods that are just trivial calls to real interface methods). This makes stubbing very clear, whereas previously trying to stub out the FooClientBase was tricky since only half the members _needed_ to be overridden (since the others were given virtual base implementations); they _could_ be overridden, but the code helpers didn't auto-fill overrides. And for stub generators (like NSubstitute/Moq/etc), I don't think it really changes much either and also makes interface breaking changes easier to not be broken by unless the interface you're mocking got updated, at which point the breaking change can't really be avoided).

I'd be sad to see a return to the old interfaces, but it won't be the end of the world if they come back. I don't think breaking changes are that scary though, it's really just test code that would need trivial updates to pick up new interface changes. I think the cases for implementing non-stub implementations of these classes would be fairly eclectic, but maybe others would find less-eclectic reasons.

For the fix for this issue, I'm leaning towards updating all the WaitForExternalEvent interface methods to take in a CancellationToken, and adding new extension methods to handle the CancellationToken-less "overloads". Yay/nay?

[AtOMiCNebula]

@ConnorMcMahon / @cgillum Are you still interested in taking this fix? It looks like you removed the v2.1.0 Release milestone, but the only reason I haven't put a PR up for this is because I haven't heard from you which approach you'd like me to take here.

[ConnorMcMahon]

We are interested in taking a fix still, we just don't think we have a way to take it in a non-breaking way for the 2.1 release this week.

Essentially, we need to make a decision about whether we can continue to use interfaces, with clear communication that we may make "breaking changes" that should only affect test code, or whether we need to add some abstract class support in order to support adding new methods like this. We will need to make this decision sooner rather than later, but it is unlikely we will be able to make it on time for 2.1.

[AtOMiCNebula]

Disappointing, but understandable. Please let me know when you make a decision.

[AtOMiCNebula]

I have prepared a fix against the v2.1.0 tag, and have pushed it here: https://github.com/AtOMiCNebula/azure-functions-durable-extension/tree/jdweiner/WaitForExternalEvent-TimeoutFixes

Happy to open a pull request once you've decided how you want to proceed with interface changes like this. I'm carrying the patch locally for now, since I didn't want to let resolving this keep delaying adoption of the new version for my product. But it'd be really great if we could come to an agreement here on how to proceed @ConnorMcMahon / @cgillum. Thanks, and happy new year. 😄

[AtOMiCNebula]

@ConnorMcMahon / @cgillum ping!

[ConnorMcMahon]

Sorry for keeping you waiting @AtOMiCNebula.

I think the stance we are taking is that we are going to allow new methods on interfaces, as it should only break test code (and only test code written with concrete implementations of the methods). It is also very simple for Intellisense to fix any of these breaking changes by making an implementation o f the new method with a "NotImplementedException" body.