Unify plugins hook dispatch #5077
Conversation
|
The debian build problem just looks like a bad git request. Probably just need to retry. But the ubuntu build failure is a core during the regression test. The stack has the hook logic on it which is worrying. |
proxy/ProxySession.cc
Outdated
| return 0; | ||
| } | ||
|
|
||
| SCOPED_MUTEX_LOCK(lock, hook->m_cont->mutex, mutex->thread_holding); |
There was a problem hiding this comment.
You changed the MUTEX_TRY_LOCK to SCOPED_MUTEX_LOCK - I don't think that's a good idea.
There was a problem hiding this comment.
Fei made me do it :/
There was a problem hiding this comment.
Just to guarantee the lock is held when calling invoke. :D
dyrock
force-pushed
the
unify_hook_dispatch
branch
from
3b3bdaf
to
b7cf5fc
Compare
|
[approve ci autest] |
dyrock
force-pushed
the
unify_hook_dispatch
branch
from
b7cf5fc
to
88be9dd
Compare
configure.ac
Outdated
| #]) | ||
| #AS_IF([test -d /opt/local/lib], [ | ||
| # TS_ADDTO(AM_LDFLAGS, [-L/opt/local/lib]) | ||
| #]) |
There was a problem hiding this comment.
Seems unrelated to the rest of the PR
There was a problem hiding this comment.
oops. That is for compiling on my mac. I'll remove it :/
dyrock
force-pushed
the
unify_hook_dispatch
branch
from
88be9dd
to
acd659b
Compare
|
[approve ci autest] |
dyrock
force-pushed
the
unify_hook_dispatch
branch
from
acd659b
to
c07f84b
Compare
|
[approve ci clang-analyzer] |
1 similar comment
|
[approve ci clang-analyzer] |
|
[approve ci autest] |
|
[approve ci clang-analyzer] |
| } | ||
|
|
||
| template <typename ID, int N> | ||
| void | ||
| FeatureAPIHooks<ID, N>::append(ID id, INKContInternal *cont) | ||
| { | ||
| if (likely(is_valid(id))) { | ||
| hooks_p = true; |
There was a problem hiding this comment.
Is there a reason to remove the likely hints? At worst, they are pretty harmless right?
|
Also this looks like an invasive change, with unclear reasons as to why this I needed. We really want this into 9.0 this late in the game ? |
|
[approve ci autest] |
|
Up to the release manager whether to pull this into 9. We have been running with this code change for several months now. |
|
It's not so late in the game, the PR has been here for quite a long time. And yes, I definitely want this in ATS 9 because it's a precursor to a lot of other work I want to get done in the ATS 10 time frame. We've been running with this internal for well over 9 months (we started production testing before this PR was created back in Feb). |
PR apache#5077 broke QUIC build
PR apache#5077 broke QUIC build
|
May I point out that I was right in questioning the sanity of including this in 9.0.x? :-P Thanks to Susan for fixing this bug (#6074), but clearly this was not well tested. |
HttpHookStateclass that manages all http hooks. This will be useful for later plugin coordination work.ProxySessionandHttpSMto useHttpHookStatefor hooks dispatchAPIHook,APIHooks, andFeatureAPIHooksThis is phase one of the Plugin Coordination project. The early phases will provide mostly debugging support.