Skip to content
This repository was archived by the owner on Aug 1, 2022. It is now read-only.

refactor(proxy): move to coco pooled storage, cancellable async everywhere #944

Merged
merged 25 commits into from
Sep 30, 2020

Conversation

kim
Copy link
Contributor

@kim kim commented Sep 21, 2020

No description provided.

Ok(self
.api
.with_storage(move |storage| storage.has_commit(&urn, oid))
.await??)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before you ask: ?? is pronounced The wat? operator, was stabilised in Rust'98, and is part of the Rust Dawallah Level II certification programme.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking forward to the interrobang RFC. ?! will be the The WAT?! operator and will indicate to code reviewers that they should move along

@kim
Copy link
Contributor Author

kim commented Sep 21, 2020

This is unfortunately very invasive, as it changes virtually everything to async.

Note that resetting the on-disk state is removed entirely, as that was done in a way very likely to cause unexplicable bugs down the road -- both on-disk state and network stack must be managed in lockstep.

@xla
Copy link
Contributor

xla commented Sep 21, 2020

This is unfortunately very invasive, as it changes virtually everything to async.

We don't accept any non-invasive PRs.

Note that resetting the on-disk state is removed entirely, as that was done in a way very likely to cause unexplicable bugs down the road -- both on-disk state and network stack must be managed in lockstep.

What do you propose in order to enable the UI integration tests to reset the states inbetween test runs?

@kim
Copy link
Contributor Author

kim commented Sep 21, 2020 via email

@xla
Copy link
Contributor

xla commented Sep 21, 2020

Is it absolutely necessary that the entire state is wiped for those tests? Like, do they depend on the keypair being used? Or could they just randomise the projects being created?

A lot of the tests depend on the fact that the monorepo is without a userr/owner -- mainly to test onboarding. To take a step back, what we want is to have state isolation between test runs to avoid spurious test failures from residue of other runs, maybe there is a saner way than resetting the entire state carried in the context.

@xla
Copy link
Contributor

xla commented Sep 21, 2020

To expand on this, the granularity between these tests varies but is mostly on the level of user flows closely modeled after figma user flows.

@FintanH
Copy link
Contributor

FintanH commented Sep 21, 2020

Do the frontend tests need to use the same proxy process or can they change between test runs? What I'm thinking is that a cypress test could consist of proxy --root .tmp/some/dir

@xla
Copy link
Contributor

xla commented Sep 21, 2020

Do the frontend tests need to use the same proxy process or can they change between test runs? What I'm thinking is that a cypress test could consist of proxy --root .tmp/some/dir

AFAIK cypress doesn't control the proxy, maybe @rudolfs can shed some light.

@xla
Copy link
Contributor

xla commented Sep 22, 2020

It looks like Context needs to own the peer handling -- then whenever we reset all the things we start a new peer backed by the new monorepo. Unfortunately JoinHandle is not canceling on drop. To remedy that I can see two strategies:

  • wrap the Peer::run in an Abortable and the context holds onto the AbortHandle
  • evaluate exposing cancellation in links Peer/PeerApi with a channel or other construct

@kim Is there any precedence for RunLopp cancellation in link?

@rudolfs
Copy link
Member

rudolfs commented Sep 22, 2020

Do the frontend tests need to use the same proxy process or can they change between test runs? What I'm thinking is that a cypress test could consist of proxy --root .tmp/some/dir

AFAIK cypress doesn't control the proxy, maybe @rudolfs can shed some light.

At the moment we start proxy first via a yarn task that also runs the tests. I think technically it would be possible to control the proxy lifecycle from within tests, but I wonder whether it would not slow down the test suite considerably because the proxy takes a while to start up?

@kim
Copy link
Contributor Author

kim commented Sep 29, 2020

Should be good now. Will add some more prose later.

@xla xla changed the title WIP companion to radicle-link#327 refactor(proxy): move to coco pooled storage Sep 29, 2020
@kim kim changed the title refactor(proxy): move to coco pooled storage refactor(proxy): move to coco pooled storage, cancellable async everywhere Sep 29, 2020
let alice_events = alice_peer.subscribe();

tokio::task::spawn(alice_peer.run(alice_state.clone(), alice_store, RunConfig::default()));
let _alice_runs = alice_peer.into_running();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nb funny semantics: we don't actually need to spawn this, just prevent it from being dropped. It also doesn't hurt to spawn it, though, so whatever people think is preferable in tests

};
let paths = coco::Paths::try_from(paths_config)?;
Some(()) = sighup.recv() => {
log::info!("SIGHUP received, reloading...");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the SIG HUP you drop gitit?

Because with --test, everything is in a temp dir, this is equivalent to a reset. Without --test, it's equivalent to, well, restarting.

};

if args.test {
let state = state.lock().await;
// TODO(xla): Given that we have proper ownership and user handling in coco, we should
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did not make any difference whether or not those were set up, so I removed it.

sender.clone().send(TimeoutEvent::SyncPeriod).await.ok();
});
}
announcer_handle.abort();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure about this, actually. Could be we need to wake the tasks when the parent gets aborted.

@@ -328,12 +347,12 @@ impl State {
/// * If no project for the `urn` was found.
/// * If the [`git::Browser`] fails.
/// * If the passed `callback` errors.
pub fn with_browser<F, T>(&self, urn: &RadUrn, callback: F) -> Result<T, Error>
pub async fn with_browser<F, T>(&self, urn: RadUrn, callback: F) -> Result<T, Error>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth considering to maintain a pool of Browsers here. Also, spawn_blocking the callback seems advisable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xla
xla previously approved these changes Sep 29, 2020
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏 👏 This is extra dope! Kill all the locks. 👏 👏 👏

🌔 ✋ 💅 🚓

Err(e) => Poll::Ready(Err(Error::Join(e))),
Ok(Err(e)) => Poll::Ready(Err(Error::Aborted(e))),
Ok(Ok(Err(e))) => Poll::Ready(Err(e)),
Ok(Ok(Ok(()))) => Poll::Ready(Ok(())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heard you like Ok's, so have some Ok's in ur Ok's.

@@ -328,12 +347,12 @@ impl State {
/// * If no project for the `urn` was found.
/// * If the [`git::Browser`] fails.
/// * If the passed `callback` errors.
pub fn with_browser<F, T>(&self, urn: &RadUrn, callback: F) -> Result<T, Error>
pub async fn with_browser<F, T>(&self, urn: RadUrn, callback: F) -> Result<T, Error>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xla xla requested a review from rudolfs September 30, 2020 07:22
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👩 📧 🌬 🐣

Copy link
Contributor

@FintanH FintanH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙇 It's good to see your hand over in the proxy ❤️

Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid!
We should update the QA docs, but I won't block on it.

@@ -20,46 +32,121 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {

let mut args = pico_args::Arguments::from_env();
let args = Args {
reset_state: args.contains("--reset-state"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this is removed, we can close: #960.
However we also need to update the QA docs: https://github.com/radicle-dev/radicle-upstream/blob/master/QA.md#prerequisites.

@kim Would you prefer to do that in this issue or should we open another one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the PR affects those bits we should address it in here imo

@kim kim merged commit ebabc6f into master Sep 30, 2020
@kim kim deleted the pooled-storage branch September 30, 2020 08:56
@rudolfs rudolfs mentioned this pull request Oct 1, 2020
kim added a commit that referenced this pull request Oct 1, 2020
Fixes a regression introduced in #944: while syncing, the other end may
not have the URLs we're interested in, or may go away in the middle of
us fetching. We should thus just ignore any errors.
kim added a commit that referenced this pull request Oct 1, 2020
Fixes a regression introduced in #944: while syncing, the other end may
not have the URLs we're interested in, or may go away in the middle of
us fetching. We should thus just ignore any errors.
kim added a commit that referenced this pull request Oct 6, 2020
As a follow-up to #944, this implements the Peer task orchestration using hand-rolled futures, in order to ensure proper lifecycle management of spawned tasks.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants