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

Fix request aborted observable #73874

Closed

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Jul 30, 2020

Description

the request.events.aborted$ observable is not working as expected the observable is completed when the request end, (when somebody read request data basically)

This is causing issue in the ingest manager plugin where we try to used that event to limit the concurrency on some routes
https://github.com/nchaulet/kibana/blob/fix-request-aborted-observable/x-pack/plugins/ingest_manager/server/routes/limited_concurrency.ts/#L44-L78

I added a test that reproduce the issue we are experiencing, and one potential solution

Not sure it's the best solution and if we should not rely on a new observable instead?

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Jul 30, 2020
@nchaulet nchaulet requested a review from a team as a code owner July 30, 2020 20:14
@nchaulet nchaulet self-assigned this Jul 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@@ -186,7 +186,7 @@ export class KibanaRequest<

private getEvents(request: Request): KibanaRequestEvents {
const finish$ = merge(
fromEvent(request.raw.req, 'end'), // all data consumed
Copy link
Contributor

@joshdover joshdover Jul 30, 2020

Choose a reason for hiding this comment

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

In my testing, this change does appear to be correct. Right now the aborted$ observable will complete prior to the response being sent back, which may cause us to lose aborted events. This appears to only happen on POST requests that define body validation because the end event is being fired when the request body is read. On GET requests, the aborted$ Observable does not complete until the connection is closed.

I propose we bundle this fix with the addition of a real completed$ event which will emit whenever the request is either aborted, or a response has been sent back to the client. Here is a PR: #73898

@nchaulet
Copy link
Member Author

Closed in favor of #73898

@nchaulet nchaulet closed this Jul 31, 2020
@nchaulet nchaulet deleted the fix-request-aborted-observable branch July 31, 2020 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants