@ShooterIT why is the loop in prepareShutdown needed?
file will be closed anyway when the process exists?
similarly, why look at shutdown_asap? why not always use bg_unlink?

@oranagra Just it is a good programming style, we closed all files we opened when process exited even OS could clean up everything as much as possible, actually, Linux can't clean somethings such as POSIX semaphores when process exits, https://man7.org/linux/man-pages/man7/sem_overview.7.html

just above reason, I want to unlink file exactly when shutdown_asap is set since process will exit soon in this case.

From my experience, cleanup code at exit can often do more harm than good (like prevent the process from exiting or delay it, thus delaying the restart and making the service unavailable).
for instance, if for some reason the bio thread is stack redis will never exit.
i'd like to see what @yossigo thinks about these.

p.s. i would rather have sigShutdownHandler call rdbRemoveTempFile with a specific force flag before doing exit rather than rely on the global (can be set to true in another scenario, before redis reaches to shutdown via serverCron)

Another thing we should care about is we only can call async-signal-safe functions from signal handlers safely, bg_unlink will call pthread_mutex_lock and zmalloc functions that are not async-signal-safem, and access global variables. What's more, server.shutdown_asap also should be volatile sig_atomic_t type, also please have a look #7778

I also like to use a force flag to specify we use unlink or bg_unlink, I used shutdown_asap just because I didn't want to change to much. But I think I am wrong, I will break encapsulation and change the original logic silently if I use the global shutdown_asap.

hi @oranagra I took a more look at rdbRemoveTempFile, I found we use snprintf in it, AFAIK, snprintf is not async-signal-safe function, right?

@ShooterIT i see that indeed snprintf isn't listed as async-signal-safe (not required to be so in POSIX), but i actually don't understand why... unlike printf which messes with stdout, sprintf is just a utility function, so maybe it is safe although not required to be.

but maybe an easier solution would be to just use sdscatfmt instead?

@oranagra I also don't know it clearly. There are some discussions, https://stackoverflow.com/questions/6758222/snprintf-in-signal-handler-creates-segmentation-fault-if-started-with-valgrind
we only use format specifiers %d, maybe it is right, but i can't certain.

We can't use sdscatfmt because it will call malloc.

I think we should just adopt a async-signal-safe implementation of a limited snprintf(), there are a few floating around and used for this purpose.

Regarding cleaning up, I agree with @ShooterIT this is a good practice but I also agree with @oranagra that we probably don't want to block a terminating process only because we're waiting for ulink() to return. I think a good middle way would be to put a time limit on waiting and produce a log message if we know we're exiting while some files have not yet been fully removed.

@yossigo why import another snprintf implementation and not just use sdscatfmt for this one case?
it might be less efficient (heap allocations), but for this minor print it's insignificant and i don't think this case justifies adding another big pile of code.

i can argue that even when rdbRemoveTempFile is called from prepare_shutdown, we may prefer to do bg_unlink and exit rather than either use blocking unlink or bg_unlink + wait loop.
although i'm not sure what are the implications (who's time will be used to actually delete the inode)

@oranagra Why do you assume zmalloc() is safe?
Regarding the other issue, the interesting question is would unlink complete in the background or will it keep the process defunct until it is completed. Even if it does, I think it may still be better than blocking before exit().

@oranagra In signal handle, we only can use async-signal-safe functions, sdscatfmt will call malloc that is not async-signal-safe.

In prepareForShutdown, If we worry about blocking, we can just use bg_unlink and exit, right?

sorry, my bad about malloc.
in theory we can maybe create a stack based sds that is large enough and know that sdscatfmt isn't gonna realloc, but that's really ugly, so i guess we need to bring another implementation of sprintf.

i prefer bg_unlink and exit (assuming the kernel will still make sure the file is deleted), seems better to me than a loop to wait till we close the file, or a blocking unlink

Hi @oranagra @yossigo I use ll2string and strncpy to replace snprintf, that references to serverLogFromHandler. And i add some tests to cover this commit. Please have a look.

I also remove waiting loop in prepareForShutdown

In order to test file still is not closed, i add sleep in bioProcessBackgroundJobs in my test code but not in commit.

@@ -199,6 +199,7 @@ void *bioProcessBackgroundJobs(void *arg) {
 
         /* Process the job accordingly to its type. */
         if (type == BIO_CLOSE_FILE) {
+            sleep(2000);
             close((long)job->arg1);
         } else if (type == BIO_AOF_FSYNC) {
             redis_fsync((long)job->arg1);

@oranagra Please review my comments in prepareForShutdown, also please squash into one commit if you feel ok

@ShooterIT i see the new test failed on MacOS, can you look into it?
https://github.com/redis/redis/runs/1146950414