Description of remove() is inaccurate

[ddorwin]

The current description of remove() is:

Removes stored session data associated with this object.

However, this method does not remove anything - it generates a release message. Any removal occurs when the response to that message is passed to update().

(The name is also somewhat inaccurate. release() or requestRemoval() might be more accurate.)

[steelejoe]

I disagree. The _release _message is simply a notification to the server of what has happened. The remove() should directly trigger any local state clearing that needs to happen. Typically the release message is used for book-keeping on the server side and would allow additional licenses to be requested (for example from other devices) but if it is not received or responded to, the client should still have destroyed the local license.

[ddorwin]

Good point - the license is destroyed and unusable. I think the description should focus on that. Specifically, the key steps are:

If any license(s) and/or key(s) are associated with the session:

Destroy the license(s) and/or key(s) associated with the session.

While the release message may not be a requirement, it is fundamental to the two types of sessions currently supported by this method. Both will leave some form of "session data" until the release is ACK'd by the update() call. Thus, "Removes stored session data" seems inaccurate. Also, in the case of "persistent-usage-record", there may not have been any stored session data - this method actually causes the record of key usage to be stored.

[mwatson2]

Perhaps we should simply adjust the description to say "Removes all license(s) and key(s) associated with the session." ?

The point of confusion is that stored session data remains after remove() is called (specifically, the record of license destruction or the record of key usage).

[ddorwin]

SGTM.

We might follow that with something about session data still being persisted until a release message is acknowledged (depending on the sessionType, though this is true for all types currently supported by this method).

[mwatson2]

Any other comments ? Or shall we move this to "Needs implementation" ?

[jdsmith3000]

How about this for replacement text under remove()?

Removes all license(s) and key(s) associated with the session. Session data will be cleared as defined for each sessionType once a release message acknowledgment is processed by update().

[mwatson2]

I'd suggest "Other session data, if any, ..." for the second sentence.

[ddorwin]

Looking at the steps in the algorithm, it destroys the license(s) and key(s). For "persistent-license", this is actually removing them from persistent storage, but for "persistent-usage-record", they are just destroyed in memory and there is nothing yet persisted to remove. One could say they were "removed from memory," but that doesn't seem like common phrasing. One could argue that this should be called destroyKeys().

@jdsmith3000's proposal with @mwatson2's suggestion SGTM, though I suggest s/Removes/Destroys/.

[ddorwin]

Reviewing #171 and related PRs led me to identify some additional algorithm issues (#180) that affect and relate directly to this issue. I believe resolution of this issue is blocked on resolution of #180.

[jdsmith3000]

Per editors review (@ddorwin, @mwatson2, @jdsmith), this issue will be resolved with the resolution of issue #180.