CancellationToken Support

[matthawley]

Curious if there's a reason why all the Async methods on IFeatureManager do not take a cancellation token? Can overloads be added that take this in and appropriately use them within the stack? If things aren't truly asynchronous, can synchronous methods be exposed?

[jimmyca15]

@matthawley

It's an oversight. Since IFeatureManager is an interface we need a breaking change release to address it. But it is known and we plan to address it when we have the opportunity to make a breaking change. Should be upcoming based off what we have in the works.

[zhiyuanliang-ms]

Hi, @matthawley
This issue will be solved in our v4 release. Currently, v4 is under review. You can check the code in preview branch.

[brental]

@zhenlan @zhiyuanliang-ms In #376 we were discussing cancellation tokens and the fact that the FeatureManager class does not currently support them came up. I was going to raise a new issue for that to address it in v4 (as part of adding cancellation token support) but found this pre-existing issue. It would be good to ensure that support gets added to the FeatureManager class and it's interfaces as part of v4.

[zhiyuanliang-ms]

@jimmyca15 @rossgrambo ISessionManager should take cancellation token too.

[brental]

@zhenlan @zhiyuanliang-ms Are these changes still planned for v4.0.0? I ask because as per #500 the v4 code has been merged into main in prep for release but the changes to address this issue have not been implemented. It would be good to have these changes in as part of the v4 major version given they would be breaking changes.

[jimmyca15]

@brental v4 adds IVariantFeatureManager (link). This interface is a superset of IFeatureManager, and it also supports cancellation token.

Since this can be used for proper cancellation, we avoided any breaking change on IFeatureManager.

[brental]

@jimmyca15 It is good that IVariantFeatureManager has the cancellation token support. However, not updating IFeatureManager and IFeatureManagerSnapshot to support it as well leaves consumers who aren't interested in the variant functionality and choose to continue using those interfaces without proper cancellation token support. So, I don't believe that the fact that IVariantFeatureManager supports it means that IFeatureManager and IFeatureManagerSnapshot should not be updated to support it as well. They are still valid ways to consume the library and should have cancellation token support. Consumers should not be forced to use IVariantFeatureManager simply to have cancellation token support. So, I think the opportunity should be taken to make the breaking change so they also have proper cancellation token support.

Additionally, that does not address the lack of cancellation token support for the ISessionManager interface. This was also identified as something that needs addressing but has not been implemented.

If the opportunity is not taken to make these breaking changes as part of v4 it further delays having proper cancellation token support in the library.

It is also a little bit frustrating that previous discussion in this issue and in #376 states that it would be addressed in v4 and it is part of the v4 milestone, but no update was made to this issue to state the changes might not be made to allow further discussion on it sooner.

[jimmyca15]

@brental

This issue should have been updated. The intention of the v4 release is to go without breaking changes.

Consumers should not be forced to use IVariantFeatureManager simply to have cancellation token support. So, I think the opportunity should be taken to make the breaking change so they also have proper cancellation token support.

I don't think it's so clear cut to say we should take a breaking change to modify IFeatureManager when IVariantFeatureManager will offer this functionality.

ISessionManager is another matter. Given we don't want to have breaking changes in v4, I might propose that we have a new interface that allows cancellation token.

[rossgrambo]

Just wanted to note- IVariantFeatureManager can directly replace IFeatureManager. There is no change in functionality to the existing methods, like IsEnabledAsync.

Another option is using FeatureManager directly to get the cancellation token methods. The direct class offers both methods with and without cancellation tokens in order to be backwards compatible, so I think IVariantFeatureManager is a better option.

[brental]

I understand wanting to avoid breaking changes but it seems like that in this case that goal is going to result in the best abstractions to use becoming less obvious. If you were looking at the library you would probably think that IFeatureManager / IFeatureManagerSnapshot would be the primary abstractions to use to check features are enabled. However, from v4 those abstractions would not be the best way to check features are enabled given that the IVariantFeatureManager / IVariantFeatureManagerSnapshot abstractions support cancellation tokens and IFeatureManager / IFeatureManagerSnapshot don't. Introducing a new interface for cancellation token support for ISessionManager could result in a similar issue (depending on how it is named). It seems like that has been discussed though and was decided as preferable to making breaking changes so whilst I don't completely agree with the decision I will leave it there.

However, if the goal is to avoid breaking changes in v4 then I think there is an issue with change to make the FeatureManagerSnapshot class take IVariantFeatureManager instead of IFeatureManager in the constructor. If a consumer is using DI and has registered a custom IFeatureManager implementation (but not a custom IFeatureManagerSnapshot implementation) then prior to v4 that implementation would be used by FeatureManagerSnapshot. When they upgrade to v4, FeatureManagerSnapshot would revert to using FeatureManager as that would be the default registered IVariantFeatureManager implementation (or throw an exception if there is no IVariantFeatureManager implementation registered). This would cause an unexpected behaviour change for those consumers. That is a separate issue to this one but needs some consideration as the consumer would have to make changes to maintain the same behaviour.

[jimmyca15]

@brental

Thank you for raising this concern. This is an overlooked change that could introduce a difference in behavior.

[jimmyca15]

@rossgrambo

[brental]

@jimmyca15 Has there been any further discussion on cancellation token support in ISessionManager (or the new interface that supports cancellation tokens)?

[zhiyuanliang-ms]

Hi, @brental The main reason why we don't add cancellation token to it now is that we want to avoid breaking changes. Do you have any specific scenario that eagerly requires ISessionManager to support cancellation token?

I don't think there is a way to avoid breaking change for this, unless we create a new interface called ISessionManagerWithCancellationToken but this looks ugly. If our decision is that finally we will support cancellation token for ISessionManager, then I think now is a good timing to take the breaking change. Otherwise, we have to wait for v5. What do you think? @rossgrambo @zhenlan @jimmyca15

BTW, we should give ISessionManager more love. At least document it. Track in #394

[rossgrambo]

BTW, we should give ISessionManager more love. At least document it. Track in #394

Agreed.
 
I think we should go with IVariantSessionManager- since it also doesn't support variants yet. It will be a similar change to the IFeatureManager vs IVariantFeatureManager change. This means it would not be a breaking change so we could add it in the next minor update.

[brental]

@zhiyuanliang-ms I don't have a specific scenario at this stage, it is more about ensuring the library has CancellationToken support in case consumers do have a need. Cancellation token support in ISessionManager was discussed previously in this issue and it was scheduled for v4 but I noticed it had not been addressed in the full release of v4 so was just following up on it.

I am aware of the desire to avoid breaking changes from previous discussions in this issue. A separate new interface was previously mentioned by @jimmyca15 to avoid the breaking change so I figured that might be the approach.

The IVariantSessionManager @rossgrambo suggested sounds reasonable because as he said it fits in with the IVariantFeatureManager.

[brental]

@jimmyca15 @zhiyuanliang-ms @rossgrambo The session manager changes are being tracked in #394 and the other required cancellation token changes have been made. So, I think this issue could probably be closed now.