Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use "drop-notifier" pattern for to asynchronously shut down containers with Http client #326

Closed
thomaseizinger opened this issue Jan 16, 2022 · 2 comments · Fixed by #575
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@thomaseizinger
Copy link
Collaborator

Not sure if this is documented anywhere but I've since learned about this very nifty pattern for doing async work in a Drop implementation:

  1. Have a Void async, oneshot channel
  2. Store the Sender in the struct you intend to eventually Drop
  3. Listen on the receiver in a task
  4. Once you get oneshot::Canceled, you know that the struct has been dropped
  5. Perform the async work

To utilize this, we need to store the Sender in ContainerAsync and the Receiver in Http. The client (Http), needs to create one task per container that it starts and shut it down as soon as it receives oneshot::Canceled.

This should allow us to drop the executor feature on futures.

@thomaseizinger thomaseizinger added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 16, 2022
@andrey-yantsen
Copy link
Contributor

Can you please help me understand the idea? I'm failing to come up with a possible implementation for a good half of what you wrote :D

To execute some async work — we'd need to spawn an async task, and to do that — we need an executor. Also, to perform a "proper" drop because of the threads and stuff, we'd need to do a join on all the spawned threads inside the Http's Drop...

What am I missing? Do you have an example of the implementation?

@thomaseizinger
Copy link
Collaborator Author

So when writing something up, I realized that actually utilizing the drop notifier here is a bit pointless :D

I'll write it up anyway because I think it is a really interested pattern.

Pattern

What this does not do is synchronize the drop with the async code that executes, i.e. Drop will complete before the async work gets done.

  1. Return a DropListener in addition to ContainterAsync from Http::run.
  2. DropListener would implement Future and probably just store a BoxFuture inside.
  3. The BoxFuture would be constructed from something like:
let client = Http {
    inner: self.inner.clone(),
};

Box::pin(async move {
	match listener.await {
		Ok(void) => void::unreachable(void),
		Err(Canceled {}) => {
			client.rm(id).await;
		}
	}
});

Returning a dedicated future makes us executor agnostic. What users would have to do is:

let (container, drop_listener) = client.run(HelloWorld {}).await;
tokio::spawn(drop_listener);

This is quite a klunky API though and with testcontainers being targeted at making tests ergonomic, this is quite the deal breaker.

There is an alternative to this where we define an Executor trait and users you can pass in an instance when you initially construct Http. That would allow us to spawn tasks onto the user's executor. However, if we do that, we might as well just call

executor.spawn(async move {
	client.rm(id).await;
})

from the container's Drop impl directly, no need to use the drop notifier pattern.

DDtKey added a commit that referenced this issue Apr 18, 2024
Quite large refactoring as part of project revamp #563, and also the long-awaited refactoring #386

Closes #386
Closes #326
Closes #475
Closes #508
Closes #392
Closes #561
Closes #559

MSRV was bumped to `1.70` in order to use `std::sync::OnceLock`. This should NOT be a problem, tests usually executed on more recent versions (also see [this ref](https://github.com/testcontainers/testcontainers-rs/pull/503/files#r1242651354)).
DDtKey added a commit that referenced this issue Apr 18, 2024
Quite large refactoring as part of project revamp #563, and also the long-awaited refactoring #386

Closes #386
Closes #326
Closes #475
Closes #508
Closes #392
Closes #561
Closes #559

MSRV was bumped to `1.70` in order to use `std::sync::OnceLock`. This should NOT be a problem, tests usually executed on more recent versions (also see [this ref](https://github.com/testcontainers/testcontainers-rs/pull/503/files#r1242651354)).
github-merge-queue bot pushed a commit that referenced this issue Apr 22, 2024
Quite large refactoring as part of project revamp #563, and also the
long-awaited refactoring #386

Now it's really simple API, e.g:
```rs
let container = GenericImage::new("redis", "latest")
        .with_exposed_port(6379)
        .with_wait_for(WaitFor::message_on_stdout("Ready to accept connections"))
        .start();
```

I find this new API much easier to use, solves a lot of problems, and
seems flexible enough to be extended.
This also works regardless of the tokio runtime flavor (multi-thread vs
current-thread). And the sync API is still available, right under
`blocking` feature (just like `reqwest` does).

From a maintainer's perspective this also simplifies the code, we don't
have to worry about two different clients and their differences.

### Docker host resolution
The host is resolved in the following order:

1. Docker host from the `tc.host` property in the
`~/.testcontainers.properties` file.
2. `DOCKER_HOST` environment variable.
3. Docker host from the "docker.host" property in the
`~/.testcontainers.properties` file.
4. Else, the default Docker socket will be returned.

### Notes
- MSRV was bumped to `1.70` in order to use `std::sync::OnceLock`. This
should NOT be a problem, tests usually executed on more recent versions
(also see [this
ref](https://github.com/testcontainers/testcontainers-rs/pull/503/files#r1242651354)).
- `Cli` client is removed, instead we provide `sync` (under `blocking`
feature) and `async` impls based on HTTP client (bollard)
- tested with
[modules](https://github.com/testcontainers/testcontainers-rs-modules-community)

## Migration guide 

- Sync API migration (`Cli` client)
  - Add `blocking` feature
  - Drop all usages of `clients::Cli`
  - Add `use testcontainers::runners::SyncRunner;`
  - Replace `client.run(image)` with `image.start()`
- Async API migration (`Http` client)
  - Remove `experimental` feature
  - Drop all usages of `clients::Http`
  - Add `use testcontainers::runners::AsyncRunner;`
  - Replace `client.run(image)` with `image.start()`

## References

Closes #386
Closes #326
Closes #475
Closes #508
Closes #392
Closes #561
Closes #559
Closes #564
Closes #538
Closes #507
Closes #89
Closes #198
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants