Hapi request events are not being triggered with basepath proxy

[lukasolson]

I've set up a server route like so:

server.route({
  path: '/api/sleep/{ms}',
  method: 'GET',
  handler: async req => {
    req.events.once('disconnect', () => console.log('disconnected'));
    const ms = parseFloat(req.params.ms);
    return new Promise(resolve => {
      setTimeout(() => {
        resolve('okay');
      }, ms);
    });
  },
});

However, the console.log statements never occur, even when the request is actually disconnected. (See https://hapijs.com/api/17.5.3#-requestevents.)

When I set up a brand new project with only Hapi/17.5.3 as the dependency and register the exact same route, then abort a request, I see the event is triggered.

[elasticmachine]

Pinging @elastic/kibana-platform

[lukasolson]

Just a note that after talking with @spalger this seems to be related to the base path proxy, as disabling it fixed the issue.

[joshdover]

It looks like the h2o2 plugin we use for the base path proxy does not expose the original request in any programmatic way, so we can't listen for connection aborts to abort the request to the proxied service. It looks like we'd need to fork and change that plugin in order to fix this.

[joshdover]

That said, this dependency is pretty small and we could probably just rip it out and replace it with what we need.

[lukasolson]

@joshdover This isn't blocking me at all, I'm moving forward with development of my feature with the base path disabled. (I played around with nginx as a proxy instead and it seems to close the connections on the upstream connection just fine, as long as you have it configured correctly.)

[watson]

FYI: We're upgrading to hapi v18 in #80468, which includes an upgrade of h2o2. It could be that this upgrade allows us to hook into the disconnect events, but I haven't checked.

[dgieselaar]

Ran into this as well today (found a Slack conversation about it by accident).

[mshustov]

still reproduced on the master

[thomasneirynck]

running into this as well. for context: Maps-backend needs to respond to request-cancellation so it can do some resource-cleanup. #90440

[joshdover]

At this point, I think it's worth letting this bug stay open until we migrate to Bazel. Bazel will allow us to experiment with using a more realistic proxy, like nginx, which should better represent real-world scenarios.

[joshdover]

After more consideration and looking at what config we need to use in the proxy, I think it's likely better for us to go ahead and fix this upstream rather than waiting for Bazel to be ready for this. This appears to be a growing issue for developers and likely is fixable.

One concern is that the h2o2 repo hasn't had any activity in quite some time. If they don't accept our patch and release a new version, we'd need to set up a separate repo or copy the source code (including tests) into Kibana. I'd prefer not to do this if possible. I think it's worth getting a fix with a PR against the upstream repo up and seeing if they accept it quickly before making a plan B decision.

[pgayvallet]

Ok, so I took a quick look.

It seems as easy as adding

request.raw.req.once('aborted', function () {
  promise.req.abort();
})

here: https://github.com/hapijs/h2o2/blob/2ac14fc746d21f97f4bb8004bc80b620d076c770/lib/index.js#L164

(and adding tests)

Currently working on an upstream PR. Will see if there is any response or if we'll have to fork / copy to our repo.

[pgayvallet]

Created hapijs/h2o2#125, now waiting for upstream

[pgayvallet]

hapijs/h2o2#125 has been merged, and v9.1.0 released. Will open a PR soon to bump the version.