Rename vague has_state_changed to has_memory_state_changed (On Hold)

[rayluo]

It caused some confusion when team members implementing token cache file state change feature in a subclass.

[marstr]

tl;dr IMHO, we should get rid of this member variable all together.

I say that because I'd rather encourage persistent TokenCache implementations to take the following semantics:

• Modification Operations:
  1. Take a lock on the resource that's going to be persisted to. (i.e. use CrossPlatLock)
  2. See if there have been any changes to the cache since the last time the resource was loaded.
     1. If there's been a change, load into memory.
  3. Modify the cache as requested.
  4. Write the resource immediately.
  5. Release the lock taken in step one.
• Read Operations:
  1. Take a lock on the resource that was persisted. (i.e. use CrossPlatLock)
  2. See if there have been any changes to the cache since the last time the resource was loaded.
     1. If there's been a change, load it into memory.
  3. Collect the queried information.
  4. Release the lock taken in step one.

In both of the iia steps, the TokenCache implementations that I've written really need a timestamp with the last time they did a write operation. However, I think that logic is specific to TokenCache implementations that are backed by a file. For instance, a SQL or Consul backed implementation (not endorsing these implementations, just examples) may have little use for such a timestamp. For that reason, I would prefer to keep that timestamp out of this shared file and put it closer to the file-backed implementations instead.

The place where I could imagine implementations needing this state-change flag: if you wanted to batch together a few token adds into a single write-operation. I'm just now sure how common that is, and it would force the write logic out of the modification method and into a stand-alone "flush" method. This would give some of the persistent implementations a slightly different interface, and I don't want to do that unless it brings real value.

That said, this falls into the category of "strong opinion, weakly held" for me. If you've got a use-case in mind for it, feel free to disregard. :)

[rayluo]

@marstr Thanks for your thoughtful input, and your sharing of that "Strong opinion, weakly held" approach!

I think your suggestion makes sense. On the other hand, for the sake of completeness, I can also explain why the current has_memory_state_changed flag was invented and why it did not reach its full potential.

The has_memory_state_changed was indeed designed for a very simplified scenario, which is to persist token cache in one way i.e. from-memory-to-disk, flush at the time of app termination, for one single app who does NOT share cache with other apps. As naive as it might seem, it is arguably the most common scenario, say, a 3rd-party script (rather than Azure CLI), being run manually or by crontab, on a trusted machine (either on end user's desktop or on a server under their control), so the corresponding implementation did not even have encryption.

That experimental implementation was not merged, due to potential security review rejection on its lacking of encryption. In the real world people are rolling their own similar implementation anyway, and they were even sending their implementation to us in hoping that we would adopt it. Currently we provide no out-of-box token persistence, but provide a how-to recipe (which uses that has_state_changed flag). Such a middle ground was never ideal in the first place, because it splits one otherwise integral behavior i.e. memory-to-disk-persistence into two incomplete pieces: (1) a vague has_memory_state_changed flag being exposed but would otherwise be an internal member, and (2) an workable (albeit naive) file cache implementation that has no plan to be shipped.

So, that is the sad life of has_memory_state_changed.

Lastly, the logic @marstr you described is for the other way around, from-disk-to-memory, which is suitable to the extension module you are currently working on. As long as we'll ship a feature-completed TokenCache sub-class, there shouldn't have a need to expose its internal flag. So, you are good.

We will keep this PR on-hold for now, and revisit/retire it later when our extension module is completed.

[rayluo]

We don't see to have a strong need for this change right now. Closing.

UPDATE: Also, downstream projects (like this one) already start picking up such public member.