Skip to content
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

Merged

Conversation

DaveCTurner
Copy link
Contributor

ActionFilter implementations may be async, so we have to keep the
request alive while the chain is running.

Closes #103952

`ActionFilter` implementations may be async, so we have to keep the
request alive while the chain is running.

Closes elastic#103952
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v8.13.0 labels Jan 5, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 5, 2024
@elasticsearchmachine
Copy link
Collaborator

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());
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@rjernst rjernst left a 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();
Copy link
Member

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?

Copy link
Contributor Author

@DaveCTurner DaveCTurner Jan 8, 2024

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.

@DaveCTurner DaveCTurner merged commit 51521e2 into elastic:main Jan 9, 2024
15 checks passed
@DaveCTurner DaveCTurner deleted the 2024/01/05/ActionFilter-ref-counting branch January 9, 2024 08:19
@DaveCTurner DaveCTurner restored the 2024/01/05/ActionFilter-ref-counting branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SecurityActionFilter performs async actions without retaining refs to requests
4 participants