fastify / fastify-reply-from

fastify plugin to forward the current http request to another server
MIT License
148 stars 76 forks source link

allow explicitly disabling request and session timeouts with 0 #368

Closed gunters63 closed 3 months ago

gunters63 commented 4 months ago

Checklist

mcollina commented 4 months ago

why is it still a draft?

gunters63 commented 4 months ago

I made a comment in the issue (https://github.com/fastify/fastify-reply-from/issues/366), I maybe should have better put it here.

The PR was still a draft because I was unsure about the missing test about disabling the sessionTimeout. I wrote a test, but it never completed and tap ran into a timeout. I don't really know what is the reason. So i left the test out.

Maybe this isn't a problem, so I'll undraft the PR now.

mcollina commented 4 months ago

Ah I see it. So a test is missing for requestTimeout? It seems the one for sessionTimeout is there?

gunters63 commented 4 months ago

Ah I see it. So a test is missing for requestTimeout? It seems the one for sessionTimeout is there?

The other way around :)

mcollina commented 3 months ago

Can you add the test about sessionTimeout?

gunters63 commented 3 months ago

Can you add the test about sessionTimeout?

I tried, but the test (which is passing correctly and reached the last line of the test function) never completes and the test runner runs into a timeout. I'll make another try.

Another thing: In request.js there is this in getHttp2Opts():

  if (!http2Opts.sessionTimeout) {
    http2Opts.sessionTimeout = opts.sessionTimeout || 60000
  }

I tried to pass a fallback value so the test doesn´t have to wait 60s to be sure there is no timeout.

It seems opts.sessionTimeout will never get set because upstream the opts come from this place near start of fastifyReplyFrom:

  const requestBuilt = buildRequest({
    http: opts.http,
    http2: opts.http2,
    base,
    undici: opts.undici,
    globalAgent: opts.globalAgent,
    destroyAgent: opts.destroyAgent
  })

where sessionTimeout doesn´t get set. I added that in my merge request.

gunters63 commented 3 months ago

Ok, I think I solved the problem with the hanging test. I added a proper disabling session timeout test now.

I explicitly clean up the request and the two Fastify instances directly after the timeout. Then the test finishes cleanly. If I leave out any of this:

  // clean up right after the timeout, otherwise test will hang
  request.cancel()
  target.close()
  instance.close()

the test hangs. Cleaning at teardown time (t.teardown) does not help here, maybe a chicken/egg problem.