Try to renew the connection upon transient error

[da-kami]

@thomaseizinger this is an attempt to fix #303

I don't think this solution would work yet but I am not exactly sure how to rearrange the code to make it work.
I thought I put a Draft PR up :)

Currently clippy complains about:

warning: value assigned to `stream` is never read
  --> swap/src/kraken.rs:46:41
   |
46 | ...                   stream = connection::new().await?;
   |                       ^^^^^^
   |
   = note: `#[warn(unused_assignments)]` on by default
   = help: maybe it is overwritten before being read?

[da-kami]

Suggested change

	// Try to renew the connection in case of websocket failure
	// Try to renew the connection in case of connection failure

[thomaseizinger]

I don't think this solution would work yet but I am not exactly sure how to rearrange the code to make it work.
I thought I put a Draft PR up :)

Currently clippy complains about:

Well clippy is right. You are over-writing the local stream variable and then returning from the closure before ever using stream again. That doesn't do anything.

Generally, I don't see a functional change in this PR. Unless I am mistaken, you just replaced the map_err with a manual match statement but the code path is still the same I think.

I am wondering why the current code doesn't work because connection::Error::Websocket errors should always be mapped to Transient. Is there maybe a codepath in backoff where it completely gives up even with Transient errors?

I am also wondering why we never see the info log about retrying.

[thomaseizinger]

I am wondering why the current code doesn't work because connection::Error::Websocket errors should always be mapped to Transient. Is there maybe a codepath in backoff where it completely gives up even with Transient errors?

Reading backoff's source code, I think I found the problem and posted up a fix here: #305

[da-kami]

I don't think this solution would work yet but I am not exactly sure how to rearrange the code to make it work.
I thought I put a Draft PR up :)
Currently clippy complains about:

Well clippy is right. You are over-writing the local stream variable and then returning from the closure before ever using stream again. That doesn't do anything.

Generally, I don't see a functional change in this PR. Unless I am mistaken, you just replaced the map_err with a manual match statement but the code path is still the same I think.

I am wondering why the current code doesn't work because connection::Error::Websocket errors should always be mapped to Transient. Is there maybe a codepath in backoff where it completely gives up even with Transient errors?

I am also wondering why we never see the info log about retrying.

😬 Yeah, my solution does not really do much.
Still wondering if the current code makes sense without trying to re-connect. But maybe I just lack the understanding of websocket connections there.

We currently map Connection and WebSocket errors to be Transient:

xmr-btc-swap/swap/src/kraken.rs

Lines 87 to 101 in 9ad2160

	/// Maps a [`connection::Error`] to a backoff error, effectively defining our
	/// retry strategy.
	fn to_backoff(e: connection::Error) -> backoff::Error<anyhow::Error> {
	use backoff::Error::*;
	match e {
	// Connection closures and websocket errors will be retried
	connection::Error::ConnectionClosed => Transient(anyhow::Error::from(e)),
	connection::Error::WebSocket(_) => Transient(anyhow::Error::from(e)),
	// Failures while parsing a message are permanent because they most likely present a
	// programmer error
	connection::Error::Parse(_) => Permanent(anyhow::Error::from(e)),
	}
	}

But we never re-establish a connection upon those transient error, but use the stream as it was before:

xmr-btc-swap/swap/src/kraken.rs

Lines 22 to 36 in 9ad2160

	async move {
	let mut stream = connection::new().await?;
	while let Some(update) = stream.try_next().await.map_err(to_backoff)? {
	let send_result = rate_update.send(Ok(update));
	if send_result.is_err() {
	return Err(backoff::Error::Permanent(anyhow!(
	"receiver disconnected"
	)));
	}
	}
	Err(backoff::Error::Transient(anyhow!("stream ended")))
	}

Wouldn't it make sense to re-connect on the stream if we get a Connection or WebWocket error or is that just not how websocket connections work?

[thomaseizinger]

But we never re-establish a connection upon those transient error, but use the stream as it was before:

That is not true.

All of the above code is within the retry closure. The very first thing that happens is establishing a connection. If we fail, we ? which bails out of the closure with an Err that is either backoff::Transient or backoff::Permanent and defines whether retry will call the closure again.

[da-kami]

But we never re-establish a connection upon those transient error, but use the stream as it was before:

That is not true.

All of the above code is within the retry closure. The very first thing that happens is establishing a connection. If we fail, we ? which bails out of the closure with an Err that is either backoff::Transient or backoff::Permanent and defines whether retry will call the closure again.

Yeah, you're right. The backoff block actually wraps the complete async block... Forget what I said 😅