Reusable Containers Feature Follow-Up

[the-wondersmith]

Creating this issue as a dedicated place to discuss the (planned) follow up work to the recently merged reusable containers feature, (continued from the original discussion).

In order:

1. env-variable control - {placeholder, will update w/ findings and thoughts shortly}
2. ryuk - @DDtKey would love to hear your thoughts here

[SorteKanin]

I've run into multiple issues with the reusable container feature. Think it'd be really cool if it was fixed though!

In particular, consider the following test code:

fn main() {}

#[cfg(test)]
mod tests {
    use anyhow::Context;
    use redis::aio::MultiplexedConnection;
    use sqlx::{PgPool, postgres::PgPoolOptions};
    use testcontainers::{ImageExt, ReuseDirective, runners::AsyncRunner};
    use testcontainers_modules::{
        postgres::Postgres,
        redis::{REDIS_PORT, Redis},
    };

    async fn test_redis() -> Result<MultiplexedConnection, anyhow::Error> {
        let redis = Redis::default()
            .with_tag("7.4.0")
            .with_reuse(ReuseDirective::Always)
            .with_container_name("redis")
            .start()
            .await
            .context("failed to start Redis container")?;
        let host_ip = redis
            .get_host()
            .await
            .context("failed to get Redis container host")?;
        let host_port = redis
            .get_host_port_ipv4(REDIS_PORT)
            .await
            .context("failed to get Redis container port")?;

        let url = format!("redis://{host_ip}:{host_port}");

        let client = redis::Client::open(url.as_str())
            .with_context(|| format!("failed to open redis client from URL {url}"))?
            .get_multiplexed_async_connection()
            .await
            .with_context(|| {
                format!("failed to establish multiplexed redis connection from URL {url}")
            })?;

        Ok(client)
    }

    async fn test_pg() -> Result<PgPool, anyhow::Error> {
        let pg = Postgres::default()
            .with_tag("18")
            .with_reuse(ReuseDirective::Always)
            .with_container_name("postgres")
            .start()
            .await
            .context("failed to start PostgreSQL container")?;

        let host_ip = pg
            .get_host()
            .await
            .context("failed to get PostgreSQL container host")?;

        let host_port = pg
            .get_host_port_ipv4(5432)
            .await
            .context("failed to get PostgreSQL container port")?;

        let url = format!("postgresql://postgres:postgres@{host_ip}:{host_port}/postgres");

        PgPoolOptions::new()
            .connect(&url)
            .await
            .context("failed to get connection pool to PostgreSQL container")
    }

    #[tokio::test]
    async fn create_redis_and_pg_1() -> Result<(), anyhow::Error> {
        let _redis = test_redis().await?;
        let _pg = test_pg().await?;

        Ok(())
    }

    #[tokio::test]
    async fn create_redis_and_pg_2() -> Result<(), anyhow::Error> {
        let _redis = test_redis().await?;
        let _pg = test_pg().await?;

        Ok(())
    }

    #[tokio::test]
    async fn create_redis_and_pg_3() -> Result<(), anyhow::Error> {
        let _redis = test_redis().await?;
        let _pg = test_pg().await?;

        Ok(())
    }
}

Make sure there are no docker containers running before trying out the reproductions below (i.e. docker ps -a is empty).

There are two major issues here:

Without container name

Try removing the calls to with_container_name and running cargo test. On my machine, I get container 'cfa9bbd...' does not expose port 5432/tcp where container cfa9bbd... is actually a Redis container, but 5432 is the Postgres port. So it seems like it first tries to create the redis container, then creates the Postgres container, but tries to reuse the Redis container as the Postgres container? This makes no sense. Also running the tests creates 3 redis containers, which seems undesired.

With container name

Running with the with_container_name calls, one test succeeds but the other two fails. This is because resuable containers are created by first checking if the container already exists, then creating it if not. But all 3 tests run past the "container already exists" check before any of the tests have actually created the containers. I.e. this is a classic TOCTOU problem.

One test manages to create the containers and succeeds, the two others get a 409 Conflict error: Docker responded with status code 409: Conflict. The container name "/redis" is already in use by container "382b3f3...". You have to remove (or rename) that container to be able to reuse that name..

I think the way reusable containers are created needs to change to avoid the TOCTOU; it should always try to create the containers (without checking if it exists already). Then, if it gets a 409 Conflict, then reuse the existing container that clearly already exists (otherwise you wouldn't have gotten a 409 Conflict error).

Note that this problem only occurs if the reusable container doesn't already exist when running cargo test. If it exists, all 3 tests succeed. I feel like the above should work from a blank slate though.

[the-wondersmith]

@SorteKanin Thank you for the deepdive here. I suspect I know what the issue actually is, and I'll do my utmost to carve out some time and get a PR up for it stat.

[SorteKanin]

No pressure and thanks for your work!

[the-wondersmith]

@SorteKanin Good news! I was right, or at least partially right - I did know what was up, and I've got it fixed on a branch right now and the tests you posted pass no problem.

For the sake of completeness though, I figured I'd run the same tests with the reuse type set to CurrentSession, and everything immediately broke. That makes me worry that I've overlooked something potentially crucial, so I'm in the process of running that down, and will update when I've figured out what's up.

[the-wondersmith]

I think I've figured it out - it looks like a race condition between the lookup call to check if an existing container matches the create request being handled and containers actually being created for a given create request. Basically, tests are/can be run in parallel, the lookup call returns an empty list for all container creation requests and then they all try to get created and only one of 'em "wins".

I stress "looks" there because it only seems to affect things when ReuseDirective is CurrentSession; ReuseDirective::Always doesn't seem to have a problem 🤔. tl;dr still chasing it down.