mhassan1 / redis-memory-server

Redis Server for testing. The server will allow you to connect your favorite client library to the Redis Server and run parallel integration tests isolated from each other.
MIT License
75 stars 9 forks source link

Stopping redis-server.exe timed out on a high capacity Windows pc #32

Open f-w opened 4 months ago

f-w commented 4 months ago

Test scripts work on my old Windows 10 PC with 24GB RAM and 4 logical CPUs. Same scripts timed out when stopping Redis server on new Windows 10 PC with 128GB RAM and 28 logical CPUs.

Timeout is hard coded to 10s. Perhaps as a mitigation making it adjustable.

I manually ran redis-server.exe and confirmed it takes about 15s to kill the process directly. Killing its parent process tree is a much quicker way to kill the process, however.

mhassan1 commented 4 months ago

The CI tests and my Windows 11 machine don't seem to have this problem. Can you try cloning this repository and running yarn, then yarn test?

f-w commented 4 months ago
$ yarn test
 PASS  src/util/__tests__/RedisBinary-test.ts
 FAIL  src/__tests__/RedisMemoryServer-test.ts (22.647 s)
  ● RedisMemoryServer › stop() › should stop redis-server and answer on isRunning() method

    Process didnt exit, enable debug for more information.

      150 |           timeout = setTimeout(() => {
      151 |             debugfn('kill_internal timeout triggered again, rejecting');
    > 152 |             reject(new Error('Process didnt exit, enable debug for more information.'));
          |                    ^
      153 |           }, timeoutTime);
      154 |         }, timeoutTime);
      155 |         _process.once(`exit`, (code, signal) => {

      at Timeout._onTimeout (src/util/RedisInstance.ts:152:20)

 PASS  src/util/__tests__/RedisBinaryDownload-test.ts
 PASS  src/util/__tests__/resolve-config-test.ts

 RUNS  src/util/__tests__/RedisInstance-test.ts

As I mentioned, the problem only happens on a high capacity Windows PC. Large RAM is likely the main contributing factor.

mhassan1 commented 4 months ago

I am curious: could there be a redis-server command line flag that would help you? I'm not sure why having a lot of RAM would make shutdown so much slower.

I will work on a PR to add a new killTimeout option to opts.instance.

f-w commented 4 months ago

Even flags such as -v or -h take >15s to complete. I believe it is a defect of redis-server.exe. Being deprecated, there is not much can do. A better mitigation than making killTimeout an option is to launch redis-server.exe via shell, get hold of the pid of the shell rather than redis-server.exe, and kill the shell process tree.

mhassan1 commented 4 months ago

I see that cross-spawn (which is used by redis-memory-server) has built-in support for a safe Windows shell (link). You should be able to take advantage of that by passing spawn: { forceShell: true }, for example:

new RedisMemoryServer({
  spawn: {
    forceShell: true
  }
})

Can you try that? If it works, we can add it to the documentation.

f-w commented 4 months ago

Neither cross-spawn nor TS gives me forceShell option but there is shell option so I tried it. Doesn't work. Multiple errors including thrown: "redis-server instance closed with code \"4294967295\"" and

TypeError: Cannot read properties of undefined (reading 'quit')`

      61 |
      62 | afterEach(async () => {
    > 63 |   await con.quit();
mhassan1 commented 4 months ago

Please try forceShell with a @ts-expect-error comment above it. If it works, I will add it to the TS type.

f-w commented 4 months ago

Doesn't work. It kills parent shell without cascading to redis-server.exe.

mhassan1 commented 4 months ago

I thought that was what you were suggesting:

launch redis-server.exe via shell, get hold of the pid of the shell rather than redis-server.exe, and kill the shell process tree

Do you have some sample code that works for you?