Fix race conditions in Regex.cc.

[ywkaras]

These changes should prevent (low-frequency) crashes that are being seen in Yahoo Prod.

[cmcfarlen]

Should this be added to 10.0.x project as well for cherry-pick?

[ywkaras]

Should this be added to 10.0.x project as well for cherry-pick?

Debatable. Any crashes would happen at shutdown, and seem infrequent (with limited testing in Yahoo Prod).

[ywkaras]

This is where the segment violation is occurring in Yahoo Prod:

trafficserver/src/tsutil/Regex.cc

Line 114 in 5f52c37

return _match_context;

[cmcfarlen]

Could RegexContext be automatic within the thread? i.e.

RegexContext* get_instance() {
  thread_local RegexContext ctx;
  return &ctx;
}

Why is the global vector of contexts necessary at all?

IIRC this was originally done because some Debug statements in destructors could come after the shutdown of the thread's regex context. But if we use Dbg in those locations, then this isn't a problem anymore?

[cmcfarlen]

Could also look at how Metrics storage lifetime outlasts the threads and see if this would work for _Data lifetime:

https://github.com/apache/trafficserver/blob/master/src/tsutil/Metrics.cc#L34-L42

[ywkaras]

Yes, it would not be a logic error in creating automatic, function scope instances of RegexContext when one is needed. The issue is whether the performance hit is tolerable.

On the other hand, this change involves multiple threads/cores hardware locking a variable in cache, which is also a potentially significant performance hit.

[ywkaras]

Could also look at how Metrics storage lifetime outlasts the threads and see if this would work for _Data lifetime:

https://github.com/apache/trafficserver/blob/master/src/tsutil/Metrics.cc#L34-L42

Because of

trafficserver/src/tsutil/DbgCtl.cc

Line 161 in 0a53513

// There is a mutex in the C/C++ runtime that both dlopen() and _cxa_thread_atexit() lock while running.

, RegexContext::get_instance() cannot cause the construction of a thread_local variable with a non-trivial destructor. The Metrics code is not limited by that restriction.

I guess I should look into changing DbgCtl:::_new_reference() so it doesn't call tag_activated() with its mutex locked.

[ywkaras]

Closing this because I think #11416 is better.