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

[NOT FOR MERGE] TSVB abort is not working #125069

Closed
wants to merge 1 commit into from

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Feb 9, 2022

Summary

I made similar changes in two places, for some reason it works in examples but doesn't work in TSVB

TSVB:
To play just open any TSVB visualization and see console.
Expected: I expect to see 'tsvb: abort is not working' in console

Screen.Recording.2022-02-09.at.4.11.49.PM.mov

Examples:

  1. Go to app /app/searchExamples/search
  2. Scroll to the end of page and execute Request from low-level client on the server
Screen.Recording.2022-02-09.at.4.09.15.PM.mov

@azasypkin
Copy link
Member

Hey @elastic/kibana-core, we're trying to figure out why request.events.aborted$ observable doesn't yield values in one of the handlers while working as expected in another. Do you have any idea what's going here?

Comment on lines +37 to +43
request.events.aborted$.subscribe(() => {
console.log('tsvb: abort is not working');
});

request.events.completed$.subscribe(() => {
console.log('tsvb: completed$ is working');
});
Copy link
Contributor

@pgayvallet pgayvallet Feb 9, 2022

Choose a reason for hiding this comment

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

The only difference I see it that in the non-working handler, you're subscribing to both observables when in the other, you're only subscribing to aborted$. Could it be the thing? (in which case it would totally be a bug on our side)

FWIW, these observables are coming from here:

private getEvents(request: Request): KibanaRequestEvents {
// the response is completed, or its underlying connection was terminated prematurely
const finish$ = fromEvent(request.raw.res, 'close').pipe(shareReplay(1), first());
const aborted$ = fromEvent<void>(request.raw.req, 'aborted').pipe(first(), takeUntil(finish$));
const completed$ = merge<void, void>(finish$, aborted$).pipe(shareReplay(1), first());
return {
aborted$,
completed$,
} as const;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@mshustov do you know why we don't have a shareReplay (or just replay) effect on aborted$ btw?

Copy link
Contributor

Choose a reason for hiding this comment

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

I expected Hapi to terminate request handling if an underlying request is aborted. But we can add shareReplay to be extra-safe. We already discussed it during the initial implementation #55061 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

The only difference I see it that in the non-working handler, you're subscribing to both observables when in the other, you're only subscribing to aborted$. Could it be the thing?

Nope, doesn't seem to change anything even if I don't subscribe to completed events.

Copy link
Member

Choose a reason for hiding this comment

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

Hey! I just noticed that Node.js sets the aborted event as deprecated. It should still work for v16... but we should look at migrating away from it.

@alexwizp
Copy link
Contributor Author

#125069 was created, this PR can be closed

@alexwizp alexwizp closed this Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants