-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retain ref to requests when running ActionFilterChain #104000
Retain ref to requests when running ActionFilterChain #104000
Conversation
`ActionFilter` implementations may be async, so we have to keep the request alive while the chain is running. Closes elastic#103952
Pinging @elastic/es-core-infra (Team:Core/Infra) |
final var request = new Request(); | ||
try { | ||
client().execute(TYPE, request, ActionListener.<Response>running(countDownLatch::countDown).delegateResponse((delegate, e) -> { | ||
assertEquals("short-circuit failure", asInstanceOf(ElasticsearchException.class, e).getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this guaranteed to always be true ? (due to the random boolean below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's ok if everything succeeds too, we're just checking that nothing else went wrong. I added a code comment in 84a194c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
@Override | ||
protected void doRun() { | ||
Thread.yield(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to mimic work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda, but really I was just using it to help make sure I had the right things covered by the tests and forgot to remove it. Gone now.
ActionFilter
implementations may be async, so we have to keep therequest alive while the chain is running.
Closes #103952