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

How to gracefully shutdown the server? (tests are failing) #83

Open
OnkelTem opened this issue May 15, 2022 · 10 comments
Open

How to gracefully shutdown the server? (tests are failing) #83

OnkelTem opened this issue May 15, 2022 · 10 comments

Comments

@OnkelTem
Copy link

OnkelTem commented May 15, 2022

I write some tests and I cannot get the proxy gracefully shutdown.

My test is very simple:

it('should start the server', async () => {
  const server = await proxy(8000);
  await delay(1000);
  await server.stop();
});

where proxy() is an async function having this:

  await server.start(port);
  return server;

Now when I run my tests as $ npm run test -- -t 'should start the server' --detectOpenHandles I get this:

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  JSSTREAM

      1 | import fs from 'fs';
      2 | import { basename, join, resolve } from 'path';
    > 3 | import { getLocal, generateCACertificate, generateSPKIFingerprint, RequestRuleBuilder } from 'mockttp';
        | ^
      4 |
      5 | import { ResolvedManifest } from './model';
      6 | import { fileExists, ManifestError, readFile, readManifest, saveFile } from './service';

      at Object.<anonymous> (node_modules/http2-wrapper/source/utils/js-stream-socket.js:6:25)
      at Object.<anonymous> (node_modules/http2-wrapper/source/proxies/h1-over-h2.js:3:24)
      at Object.<anonymous> (node_modules/http2-wrapper/source/index.js:13:5)
      at Object.<anonymous> (node_modules/mockttp/src/rules/requests/request-handlers.ts:8:1)
      at Object.<anonymous> (node_modules/mockttp/src/rules/requests/request-rule.ts:10:1)
      at Object.<anonymous> (node_modules/mockttp/src/server/mockttp-server.ts:28:1)
      at Object.<anonymous> (node_modules/mockttp/src/main.ts:2:1)
      at Object.<anonymous> (src/proxy.ts:3:1)
      at Object.<anonymous> (src/proxy.test.ts:1:1)

What can I do about this?

P.S. Here is my proxy code: https://github.com/OnkelTem/wmod-proxy/blob/master/src/proxy.ts just for reference.

@pimterry
Copy link
Member

That's surprising. It sounds like this is caused by some HTTP/2 connection that's staying open. HTTP/2 support is a bit experimental in places, and I can definitely believe that there's some cases where pooled connections aren't shut down as aggressively as intended. I'd definitely like to tighten up this kind of thing though.

That said, it's a bit unusual that HTTP/2 is being used here at all. Because it's experimental, Mockttp uses ALPN to actively avoid it where possible - by default it's only used for clients who connect and say they only support HTTP/2, not HTTP/1 (those exist, notably in clients using gRPC, but it's not common). One quick fix option might be to just disable HTTP/2 - you can do that by passing a http2: false option to getLocal() (docs). Is HTTP/2 important for your setup?

@OnkelTem
Copy link
Author

Is HTTP/2 important for your setup?

I've successfully disabled it using http2: false option and the problem seems to went away. Thank you.

I hope you'll resolve it!

@pmpak
Copy link

pmpak commented Jun 3, 2022

I was trying quite a lot to make this error go away until I stumbled upon this issue. Setting {http2: false} worked like a charm. thanks a lot !

@jagu-sayan
Copy link

I got this issue with jest version 28.1.1 and node 16.15.1. Even with http2: false option 🤔

@pimterry
Copy link
Member

pimterry commented Jun 9, 2022

Hi @jagu-sayan, can you share the full 'open handles' error from Jest, and some code to reproduce what you're seeing, as in the original description above?

@matAlmeida
Copy link

matAlmeida commented Oct 5, 2022

@pimterry
I'm facing that same issue, here are some example code that I can reproduce the error

Note that even using the option http2 as false it hits the http2-wrapper

*My tests are passing but it shows as an warn at the end

The Error (using jest --detectOpenHandles)

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  JSSTREAM

      1 | import { faker } from '@faker-js/faker';
    > 2 | import { getLocal as mockttpGetLocal } from 'mockttp';
        | ^
      4 | import URL from 'url';
      3 | import URL from 'url';
      5 | import https from 'https';

      at Object.<anonymous> (node_modules/mockttp/node_modules/http2-wrapper/source/utils/js-stream-socket.js:6:25)
      at Object.<anonymous> (node_modules/mockttp/node_modules/http2-wrapper/source/proxies/h1-over-h2.js:3:24)
      at Object.<anonymous> (node_modules/mockttp/node_modules/http2-wrapper/source/index.js:13:5)
      at Object.<anonymous> (node_modules/mockttp/src/rules/requests/request-handlers.ts:8:1)
      at Object.<anonymous> (node_modules/mockttp/src/rules/requests/request-rule.ts:10:1)
      at Object.<anonymous> (node_modules/mockttp/src/server/mockttp-server.ts:31:1)
      at Object.<anonymous> (node_modules/mockttp/src/main.ts:2:1)

The code

import { faker } from '@faker-js/faker';
import { getLocal as mockttpGetLocal } from 'mockttp';
import URL from 'url';
import http from 'http';
import https from 'https';

export class SyntheticRequesterError extends Error {
  public request: any;

  constructor(message?: string, request?: any) {
    super(message);

    this.name = 'RequesterError';
    this.request = request;
  }
}

type RequesterCallback = (response: any) => any

export interface SyntheticRequesterDatasource {
  get(url: string, options?: https.RequestOptions): Promise<any>
}

export class SyntheticRequesterDatasourceImpl implements SyntheticRequesterDatasource {
  constructor(private baseUrl = '', private callback: RequesterCallback = (args) => args) { }

  public async get(url: string, options?: https.RequestOptions) {
    const response = await this.requester('GET', this.baseUrl.concat(url), undefined, options);

    return this.callback(response);
  }

  private requester = (method: string, url: string, data: any = '', options: https.RequestOptions = {}) => new Promise((resolve, reject) => {

    const { hostname, protocol, path, port } = URL.parse(url);

    const rawData = typeof data === 'object' ? JSON.stringify(data) : data;

    const requestData: https.RequestOptions = {
      ...options,
      path,
      method,
      hostname,
      protocol,
      port,
      headers: {
        ...options.headers,
        'Content-Length': rawData.length,
      },
    };

    const request = (protocol === 'https:' ? https : http).request(requestData, (response) => {

      let data = '';
      response.on('data', chunk => data += chunk);

      response.on('end', () => {
        let parsedData;

        try {
          parsedData = data ? JSON.parse(data) : data;
        } catch (error) {
          parsedData = data;
        }

        if ((response.statusCode || 500) >= 400) {
          const error = new SyntheticRequesterError(response.statusMessage, parsedData);
          reject(error);
        }

        resolve(parsedData);
      });
    });

    request.on('error', error => reject(error));

    if (rawData) {
      request.write(rawData);
    }

    request.end();

  });
}

describe('SyntheticRequesterDatasource', () => {
  describe('http', () => {
    const mockHttpServer = mockttpGetLocal({
      http2: false,
    });
    const mockHttpServerPort = 8080;
    mockHttpServer.start(mockHttpServerPort);

    beforeEach(() => {
      mockHttpServer.reset();
    });

    afterAll(() => {
      mockHttpServer.stop();
    });

    const baseUrl = `http://localhost:${mockHttpServerPort}`;
    const syntheticRequesterDatasource: SyntheticRequesterDatasource =
      new SyntheticRequesterDatasourceImpl(baseUrl);

    it('get 200', async () => {
      const endpoint = faker.random.word();
      const expectedResponse = faker.random.words(5);

      const endpointMock = await mockHttpServer.forGet(`/${endpoint}`).thenReply(200, expectedResponse);

      const response = await syntheticRequesterDatasource.get(`/${endpoint}`);

      expect(response).toBe(expectedResponse);

      const requests = await endpointMock.getSeenRequests();
      expect(requests.length).toBe(1);
      expect(requests[0].url).toBe(`${baseUrl}/${endpoint}`);
    });
  });
});

@pimterry
Copy link
Member

pimterry commented Oct 7, 2022

@matAlmeida can you share a complete repo or some other directly runnable demo to reproduce this? I'm not sure how to set up Jest etc to run the code shown here, and it's useful to know the exact versions for the dependency tree, and exactly how the tests are being run, and so on.

It might be worth testing different http2-wrapper subdependency versions, it's possible something has changed there that is triggering this now.

That said, I'm very surprised that http2-wrapper is being used at all in this case, since it's only ever used when an incoming request to Mockttp uses HTTP/2 (the only place it's used is here behind a check for that) and the code shown above definitely doesn't make an HTTP/2 request, whether it's disabled or not because it just uses the http and https modules, it doesn't reference http2 at all.

@matAlmeida
Copy link

@pimterry

Here is a repo with the above example: https://github.com/matAlmeida/example-mockttp

Just run npm install then npm test

@pimterry
Copy link
Member

pimterry commented Oct 7, 2022

Ah, right, it's failing here: https://github.com/szmarczak/http2-wrapper/blob/51eeaf59ff9344fb192b092241bfda8506983620/source/utils/js-stream-socket.js#L6.

This is run on import of that module, not when HTTP/2 is actually used, which is why the options don't matter. It's just caused by that module independent of Mockttp, this does the same thing:

import * as h2Wrapper from 'http2-wrapper';

describe('Test', () => {
  it("runs a test", () => {
    h2Wrapper // Just a reference so it's not elided - we don't call anything
  });
});

This is pretty clearly a Jest bug imo. In either case, this is not an open handle, it's a socket that never even connects to anything, and it's certainly not keeping node running (its easy to check - if you create an empty script that just requires that module, it exits just fine).

In a similar way, this also reports an open handle:

const tls = require('tls');

describe('Test', () => {
  it("runs a test", () => {
      const socket = new tls.TLSSocket();
      socket.destroy();
  });
});

I think this is probably the relevant Jest bug: jestjs/jest#11665. You should chase them there to get that fixed. It is possible that fixing that TLS issue alone won't be enough (since in this case it complains about TLSWRAP, not about JSSTREAM) but it's just a symptom of the same thing: Jest is making broken assumptions about Node. They need to fix that.

@matAlmeida
Copy link

Great, it makes sense. As I said, my tests was exiting without the need of forceExit from jest so i think that this proves that there were no true open handles, just a false alert from jest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants