-
Notifications
You must be signed in to change notification settings - Fork 87
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
Comments
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 |
I've successfully disabled it using I hope you'll resolve it! |
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 ! |
I got this issue with jest version 28.1.1 and node 16.15.1. Even with |
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? |
@pimterry Note that even using the option *My tests are passing but it shows as an warn at the end The Error (using jest --detectOpenHandles)
The codeimport { 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}`);
});
});
}); |
@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 |
Here is a repo with the above example: https://github.com/matAlmeida/example-mockttp Just run |
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. |
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. |
I write some tests and I cannot get the proxy gracefully shutdown.
My test is very simple:
where
proxy()
is an async function having this:Now when I run my tests as
$ npm run test -- -t 'should start the server' --detectOpenHandles
I get this: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.
The text was updated successfully, but these errors were encountered: