Deprecate TSSslSecretGet (not thread safe). #8862
Conversation
|
Looks good. It's good to give a heads-up to people that this function signature will be changing in 10.x. I suggest providing a brief description of the new signature now in these comments. |
|
The description is available in the Rel 10 documentation though. |
That's a good point. I would point to that doc, then. Something like: My intention is to provide the user with something actionable. Usually deprecation statements say, "This is the old interface and will only be supported for a limited amount of time. We encourage you to move to this new API in preparation for that." In this case, the new API will only come in the future release when this one is replaced, which is fine. But at least we can point the user to what the new API will be so they can prepare for it. But that's just my two cents, and maybe misplaced. I haven't "requested changes" intentionally. Someone else will have to approve anyway since I'm also with Yahoo. Thanks! |
|
Do we deprecate things on minor releases? I feel like we've been doing it only on major releases but I'm not sure. |
Right, most deprecation warnings point people to an updated API to use. This is a different type of deprecation warning. In this case, we're fixing a race condition with TSSslSecretGet, but we have to change the signature to do so. That is, the function name will be the same, but the return type and some of the arguments will change. Such a change is backwards incompatible. For this reason, we're putting off the fix till 10.x. Walt is alerting people of this deprecation with this PR. My suggestion to point to the new 10.x documentation with the updated signature is my attempt to clarify this. |
|
Is the change #8790? It isn't even approved. Do we have consensus on the way of depreciation, which only changes the return type and the arguments? I also found that |
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
I'm not sure if this deprecation is needed in the first place.
The typo should be fixed anyway.
I was told not to mark #8790 for back port to 9.2. If #8790 is rejected, we will have to eliminate or replace TSSslSecretGet() in another PR, it is not thread safe. #8790 is currently running in Yahoo prod, I was waiting for it to soak for a while before trying to merge it in open source. |
The current version is not thread safe because it returns a pointer into a mutex-protected data structure, which may be invalidated by another thread.
I wonder why. Backporting it to 9.2 is another option indeed. |
@bryancall do we want to reconsider back porting? |
| /* Update the transient secret table for SSL_CTX loading */ | ||
| tsapi TSReturnCode TSSslSecretSet(const char *secret_name, int secret_name_length, const char *secret_data, int secret_data_len); | ||
| /* NOTE: TSSslSecretGet() is deprecated, it is not thread-safe. It will be replaced with a thread-safe function | ||
| ** in Release 10 */ |
There was a problem hiding this comment.
Shouldn't we use TS_DEPRECATED ?
|
This won't be needed if #8854 makes it into 9.2. |
|
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community. |
5 participants
The current version is not thread safe because it returns a pointer into a mutex-protected data structure, which may be invalidated by another thread