kstyrc / embedded-redis

Redis embedded server for Java integration testing
854 stars 371 forks source link

please Expose redis process path #32

Closed n0mer closed 9 years ago

n0mer commented 9 years ago

currently in redis.embedded.util.JarUtil#extractExecutableFromJar()

import com.google.common.io.Files;
//skip
public class JarUtil {

    public static File extractExecutableFromJar(String executable) throws IOException {
        File tmpDir = Files.createTempDir();

Then in redis.embedded.RedisServer

    public RedisServer(Integer port) throws IOException {
        super(port);
        File executable = JarUtil.extractExecutableFromJar(RedisRunScriptEnum.getRedisRunScript());

then in redis.embedded.AbstractRedisInstance

    public synchronized void start() throws EmbeddedRedisException {
        if (active) {
            throw new EmbeddedRedisException("This redis server instance is already running...");
        }
        try {
            redisProcess = createRedisProcessBuilder().start();

and

    private ProcessBuilder createRedisProcessBuilder() {
        File executable = new File(args.get(0));
        ProcessBuilder pb = new ProcessBuilder(args);
        pb.directory(executable.getParentFile());
        return pb;
    }

So, as you can see, there is no way to read "executable" path outside of AbstractRedisInstance.

Is it possible to expose "executable" via getter at AbstractRedisInstance?

Sometimes (at least on windows) redis-server-64.exe is still running. I'm going to at least print the path of the executable to log, to be able to see later whether this subdirectory inside "java.io.tmpdir" was actually removed or not on exit.

kstyrc commented 9 years ago

Of course it should. However, maybe we should rather do proper cleanup of the redis process after cleanup? Under what circumstances the redis stays up after execution?

n0mer commented 9 years ago

i use Redis embedded in integration tests. spring context is managed by spring-boot, so redis cfg is the following:

    @Bean(destroyMethod = "stop")
    RedisServer redisServer() {

//...
        try {
            RedisServer redisServer = new RedisServer(port);
            redisServer.start();
            return redisServer;
        } catch (IOException e) {
//...
        }
    }
}

Every JUnit test currently uses @DirtiesContext (i doubt it is the best practice, but nothing better came to mind mind so far :) ) :

@RunWith(SpringJUnit4ClassRunner.class)
@SpringApplicationConfiguration(classes = {
        EmbeddedRedisServerConfig.class,
        JedisClientConfig.class
})
@DirtiesContext
public class RedisRepositoryTest {
//...

There is another c-tor for RedisServer that allows to specify the File for redis executable. But again, how can we specify that if there is no way to obtain tmp.path from previous invocation :) So you need to "allow" redis-embed to start in windows firewall every run, manually, for every integration test you currently have.

On windows desktop (jdk-8u25, win7 64bit), in sysinternal's process explorer, sometimes child redis-server.exe process (started from java process) stays alive even when it's former parent process is already finished. So, to avoid the exception or "redis alredy started", you need to always check manually in process explorer whether already running redis-server instance exists. Then, if any - manually kill it, and only after that re-start integration test(s) from IDE.

Maybe the cause is how actually "Ctrl+C" works in cmd.exe, in gradlew.bat, or how this SIGTERM is passed in IntelliJ-wrapped JUnit execution to java application being tested when you press "Stop" red cross in IDE, to stop your spring-boot application.

Even more - exception that is thrown when redis is already running, is nothing but informative.

n0mer commented 9 years ago

In other words, and to put it short:

kstyrc commented 9 years ago

Think it's doable and I'll address that in the next release. Would you like to make contribution? Apart of that, what is the reason redis process keeps running? It should be wiped out on JVM exit. Maybe we should treat it as a bug & fix rather than expose lib internals?

Thanks for the input.

n0mer commented 9 years ago

Sorry, i've shared all i know so far. Since then i removed redis-embedded from the project and not using it, because of this hassle of running integration tests on dev machine.

There is no way to workaround this process issue in build scripts, because tmp folder name is neither exposed nor explicitly logged. So, quickest way, for now, is to expose folder directory. And then dig & troubleshoot every possible combination of causes why the process still runs even when there is no parent any more.

Of course it's up to you.

Please ping me if i can assist in testing and verifying fix this issue. I cannot promise to provide test project with isolated test case, but will try to run updated version of redis-embed locally.

kstyrc commented 9 years ago

As you can see, on the main page of the project, we currently have CI running for Windows on AppVeyor. It truly fails on Windows. My guess is that redis process does not exit correctly and/or stales and takes up port. Will try to address & fix it soon.

kstyrc commented 9 years ago

Will try to fix embedded-redis subsequent runs in #40. If not, then I'll expose redis process path.