Redis 6 on Mac won't halt on SIGTERM if the temp directory has been deleted

[DrHyde]

If you create a Test::RedisServer object which doesn't get destroyed until global destruction, and use the default setting of having T::RS create a temp dir for you, then, because the order of destruction is not guaranteed, the directory object may get destroyed (triggering deletion of the directory) before T::RS sends the SIGTERM signal to redis-server.

Redis-server then doesn't stop, because for some reason it seems to want the directory to exist.

I have a work-around in my code, where I supply a temp dir to the constructor, and then call ...->stop in and END block, thus guaranteeing that T::RS stops redis-server while the directory still exists.

I can't think off the top of my head of a better solution which could be applied inside T::RS itself, except perhaps that of sending a SIGKILL instead, but that's a bit icky.

I've only seen this on MacOS, so it may be a platform-specific quirk of Redis - it's definitely a Redis bug at heart and I have also reported it to them: redis/redis#10330

[Songmu]

Thanks for the report. I have also seen the report to Redis itself and understand the situation.

There are several possible ways to do this.

1. Try SIGTERM a few times, and if it doesn't work, send SIGKILL.
   • Good: This is a common practice when stopping a daemon process
   • Good: It is relatively easy to implement.
   • Bad: It's a bit rough.
   • Good: However, it will definitely stop the process, not just in this case.
2. Check for the existence of the directory when stopping, and if it is not found, perform CONFIG SET appendonly no, CONFIG SET save "", and then stop the process (SIGTERM).
   • Good: This is the most correct method for this case.
   • Bad?: It's a good idea to set appendonly as well, but I can't guarantee that it will work on versions earlier than 1.1 that don't support it.
     • However, I don't think we should guarantee that older versions will work.
   • Bad: We need to add more runtime dependencies such as Redis.pm to issue commands to redis.
3. Don't use CLEANUP => 1 for implicit tmpdir creation in new, delete tmpdir by itself on stop.
   • Good?: Delete tmpdir on stop only if it is not passed to the constructor. In this case, if we explicitly pass a tmpdir with CLEANUP => 1 to the constructor, we may face the same problem as in this issue, but I don't think there are many use cases for that.
     • It's not a good idea to delete tmpdir all together, because sometimes we don't want it to be deleted for exapmle debugging purposes
   • Bad: It's a work-around, and it's tricky to read and understand the code.

Let me think some more about what to do. Your pull requests are also welcome.

[Songmu]

I have implemented the above 2 methods. I was also able to write a test that reproduced the situation, so I was able to solve the problem. ref. #15

[DrHyde]

Thanks. I've tested with both Redis 5 and 6, and both work just fine now.