[NP] KibanaRequest provides request abortion event #55061

mshustov · 2020-01-16T16:05:51Z

Summary

Closes #44302
Provides an alternative to hapi request.disconnect event, which uses aborted under the hood.
The usage example in the Legacy platform

kibana/src/legacy/core_plugins/elasticsearch/lib/abortable_request_handler.js

Line 29 in f13adfa

req.events.once('disconnect', () => {

The aborted$ event provided via Observable interface as proposed in #44302 (comment)

to provide a unified interface for all updateable data sources
to manage subscription mechanism under the hood

Usage example:

router.get({ path: '/', validate: false }, async (context, request, res) => {
  const controller = new AbortController();
  request.events.aborted$.subscribe(controller.abort);

  const client = context.core.elasticsearch.dataClient
  return res.ok({
    body: await client.callAsCurrentUser('endpoint', {}, {signal: controller.signal});
  });
});

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

KibanaRequest provides aborted$ observable that emits once if and when an incoming request has been aborted.

Usage example:

router.get({ path: '/', validate: false }, async (context, request, res) => {
  const controller = new AbortController();
  request.events.aborted$.subscribe(controller.abort);

  const client = context.core.elasticsearch.dataClient
  return res.ok({
    body: await client.callAsCurrentUser('endpoint', {}, {signal: controller.signal});
  });
});

mshustov · 2020-01-17T12:00:10Z

src/core/server/http/router/request.ts

+  }
+
+  private getEvents(request: Request): KibanaRequestEvents {
+    const end$ = fromEvent<void>(request.raw.req, 'end').pipe(first());


I added end$ to complete aborted$ observable to prevent memory leaks. I use the end event here to make the tests green https://github.com/elastic/kibana/pull/55061/files#diff-7ffe36ae4fa33e0fddf01a3182306eedR47
From my understanding end cannot be used here, as it's emitted when a request payload is fully consumed. It seems that I should use close instead, but switching to close fails the tests (end$ is not completed). I have a question: What is the correct way to understand that request won't emit anymore to complete aborted$?

Note: I don't set connection: keep-alive/close explicitly in tests, I assume the code should work for either case.

@elastic/kibana-platform @watson any thoughts?

I'm not that familiar with rxjs, but I'll do my best to answer.

You stepped right into the most complicated part of Node.js 😅 Streams can "end" in many ways unfortunately, it differs with the type of stream, and in some cases also with the version of Node.js, and it's often hard to implement correctly without memory leaks - as you also found out.

A normal request/response event-lifecycle looks something like this:

In case the underlying socket is destroyed the order of events are like so:

Note: If I want to be sure to be notified when a stream ends I always use the end-of-stream module.

Pipe only cares about the end event, so if you pipe source into target, but source then emits a close event, target will not know about it and the pipe will still be open:

Similar to the end-of-stream module, there's a pump module which takes care of this edge-case, so that the target stream is correctly closed if the source stream closes. I think that's the issue you're seeing

Note: I don't set connection: keep-alive/close explicitly in tests, I assume the code should work for either case.

Correct, the events on the request and response objects shouldn't be affected by the Connection header.

Great explanation, really appreciate it!
I surprised to see Server: Socket: closed in both your examples. From my understanding server should keep the socket open since HTTP/1.1 uses connection: keep-alive by default. What I missed here?

https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html

A significant difference between HTTP/1.1 and earlier versions of HTTP is that persistent connections are the default behavior of any HTTP connection. That is, unless otherwise indicated, the client SHOULD assume that the server will maintain a persistent connection, even after error responses from the server.

What I missed here?

In my example, I used the default global HTTP Agent in Node.js which doesn't keep connections alive. If I set it to keep-alive, it looks like this:

@watson is this built-in nodejs logger?

Nope, I'm using my own event debugging module: https://www.npmjs.com/package/event-debug

Then end usage in the code looks correct:

server handles an incoming request and responds. end emitted --> aborted$ completes --> end$ completes

an incoming request aborted / socket destroyed. aborted emitted --> aborted$ completes --> end$ unsubscribes from end event (there are no other subscriptions).

Therefore memory a memory leak should not be possible.
Additionally, I performed a loading test locally and everything looks good. load test
Using additional package https://github.com/mafintosh/end-of-stream looks a bit overkill for such a small use-case.
@elastic/kibana-platform are you okay with the current implementation or we should consider exposing the EventEmitter interface instead of Observables? Then we needn't to think about cleaning up after manually created Observable (GC removes all listeners)

elasticmachine · 2020-01-17T13:07:19Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet

implementation is looking good to me and follow consensus of #44302

pgayvallet · 2020-01-20T11:15:32Z

src/core/server/http/router/request.ts

+    return {
+      aborted$: fromEvent<void>(request.raw.req, 'aborted').pipe(first(), takeUntil(end$)),
+    } as const;


I'm wondering if we should add areplay effect. Is there a risk for the request to already be aborded when entering the request handler?

That's a tricky part... the underlying framework should handle it. How to test it? No clue. We can add sharedReplay to be double-safe.

src/core/server/http/integration_tests/request.test.ts

src/core/server/http/router/request.ts

kibanamachine · 2020-01-22T11:08:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8607136

History

💚 Build #21439 succeeded 19cb1f3
💚 Build #21328 succeeded 45142cf
💚 Build #21032 succeeded a48c47f
💔 Build #20867 failed 02344aa

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

* add aborted$ observable to KibanaRequest * complete observable on request end * update docs * update test suit names * always finish subscription * address comments

…t-of-legacy * 'master' of github.com:elastic/kibana: Restore 200 OK + response payload for save and delete endpoints (elastic#55535) [SIEM] [Detection Engine] Log time gaps as failures for now (elastic#55515) [NP] KibanaRequest provides request abortion event (elastic#55061)

* add aborted$ observable to KibanaRequest * complete observable on request end * update docs * update test suit names * always finish subscription * address comments

lukasolson · 2020-02-27T17:33:46Z

Any possibility to backport to 7.6.1? (See #58749)

* add aborted$ observable to KibanaRequest * complete observable on request end * update docs * update test suit names * always finish subscription * address comments

mshustov · 2020-02-28T09:37:38Z

@lukasolson #58851 backport to 7.6 branch. which version label I should use? 7.6.1/7.6.2?

* add aborted$ observable to KibanaRequest * complete observable on request end * update docs * update test suit names * always finish subscription * address comments

mshustov added 3 commits January 16, 2020 17:03

add aborted$ observable to KibanaRequest

02344aa

complete observable on request end

98a5e5c

update docs

a48c47f

mshustov commented Jan 17, 2020

View reviewed changes

mshustov added Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Jan 17, 2020

mshustov added the v7.7.0 label Jan 17, 2020

mshustov changed the title ~~[NP] KibanaRequest provides request abortion event~~ [PoC] [NP] KibanaRequest provides request abortion event Jan 17, 2020

mshustov added 3 commits January 19, 2020 14:25

Merge branch 'master' into issue-44302-abort-events

a934445

update test suit names

983e2be

Merge branch 'master' into issue-44302-abort-events

45142cf

mshustov marked this pull request as ready for review January 20, 2020 09:07

mshustov requested a review from a team as a code owner January 20, 2020 09:07

pgayvallet reviewed Jan 20, 2020

View reviewed changes

mshustov changed the title ~~[PoC] [NP] KibanaRequest provides request abortion event~~ [NP] KibanaRequest provides request abortion event Jan 20, 2020

always finish subscription

19cb1f3

mshustov requested review from a team and pgayvallet January 20, 2020 16:59

joshdover approved these changes Jan 21, 2020

View reviewed changes

src/core/server/http/integration_tests/request.test.ts Show resolved Hide resolved

src/core/server/http/router/request.ts Outdated Show resolved Hide resolved

lukasolson mentioned this pull request Jan 21, 2020

[Search service] Integrate with aborted$ event to cancel discarded requests #55490

Closed

mshustov added 2 commits January 22, 2020 10:40

Merge branch 'master' into issue-44302-abort-events

5ed4fc8

address comments

8607136

mshustov merged commit 3e69ea5 into elastic:master Jan 22, 2020

mshustov deleted the issue-44302-abort-events branch January 22, 2020 12:35

mshustov added the v8.0.0 label Jan 22, 2020

mshustov mentioned this pull request Jan 22, 2020

[7.x] [NP] KibanaRequest provides request abortion event (#55061) #55538

Merged

mshustov added the backported label Jan 22, 2020

lukasolson mentioned this pull request Feb 27, 2020

Preserve the original error name instead of returning raw AbortError #57550

Merged

2 tasks

mshustov mentioned this pull request Feb 28, 2020

[NP] KibanaRequest provides request abortion event (#55061) #58851

Merged

lukasolson added the v7.6.1 label Feb 28, 2020

mshustov mentioned this pull request Jul 31, 2020

Fix aborted$ event and add completed$ event to KibanaRequest #73898

Merged

7 tasks

mshustov mentioned this pull request Feb 9, 2022

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

Closed

[NP] KibanaRequest provides request abortion event #55061

[NP] KibanaRequest provides request abortion event #55061

Uh oh!

Conversation

mshustov commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev Docs

Uh oh!

mshustov Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

watson Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

mshustov Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

watson Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

mshustov Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

watson Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

mshustov Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jan 17, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

mshustov Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jan 22, 2020

💚 Build Succeeded

History

Uh oh!

lukasolson commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshustov commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mshustov commented Jan 16, 2020 •

edited

Loading

mshustov Jan 17, 2020 •

edited

Loading

mshustov Jan 20, 2020 •

edited

Loading

mshustov Jan 20, 2020 •

edited

Loading

lukasolson commented Feb 27, 2020 •

edited

Loading

mshustov commented Feb 28, 2020 •

edited

Loading