0.3 to 0.6 Migration Feedback

[casey]

I recently migrated a project I'm working on from twilight 0.3 to 0.6, and wanted to provide some feedback on the process. I mentioned some of these things on Discord, but wanted to also expound on them here in more detail. I'm sorry for not following the development of twilight more closely, and that this isn't as timely as it could be.

Pain Points

Overall the migration was a bit tricky. A few pain points:

• When using 0.3, we had relied on being able to call Cluster::some_events multiple times. For example, after bringing a cluster up, we would do:

  cluster
    .some_events(EventTypeFlags::READY)
    .next()
    .await
    .expect("Did not receive ready event");

  To wait for the ready event, before eventually calling Cluster::some_events later in the program to get events for the main loop. Adapting to getting the Events iterator when building the Cluster was a bit tricky due to the structure of our code. For testing, we have to have a single Cluster instance that's shared between multiple tests, otherwise we get rate-limited when connecting the cluster. Because of this, we have to store the Cluster in a static variable, and our intialization code is pretty convoluted.

  Overall this wasn't so bad. Changing the initialization code was a bit tricky, but I don't anticipate further challenges.

  One thing I do wonder about, is that it's now no longer possible to create multiple event streams from the same cluster that return different event types. This is totally academic, since we don't need it, but I do wonder if it's desirable functionality to have.

• Instead of being able to call .await on a request builder, we now have to call .exec. This isn't so bad, since it's a compile-time error to try to call .await, so it isn't possible to forget to do it. However, it does make the code more verbose. Would it be possible to provide Future implementations for requests that simply forward to .exec?

• Instead of returning the model that was requested, requests now return a Response, and it's necessary to call .model on the response to get the model. This isn't a huge problem, since it's not possible to forget, but it does make the code more verbose. This is particularly pronounced in our test code, where we use .unwrap instead of ?:

  let user_id = client
    .current_user()
    .exec()
    .await
    .unwrap()
    .model()
    .await
    .unwrap()
    .id;
  
  let user = client
    .user(user_id)
    .exec()
    .await
    .unwrap()
    .model()
    .await
    .unwrap();
  
  let guilds = client
    .current_user_guilds()
    .exec()
    .await
    .unwrap()
    .models()
    .await
    .unwrap();

• If a request returns multiple items, it's required to call .models instead of .model. This isn't a huge problem, since it's not possible to call the wrong method. However, it's easy to forgot and the error in that case is fairly inscrutable. I personally think of both a Foo and a List<Foo> as being a "model", so it's hard to remember that I need to call .models. Would it be possible to make .model work alongside .models?

• Since error types are no longer enums, and ErrorKind fields are private, matching on specific error types is much more verbose.

  As an example, here's a match statement that checks for a 404 error:

  match self.await {
    Ok(response) => Ok(Some(response.model().await?)),
    Err(error) => {
      if let twilight_http::error::ErrorType::Response { status, .. } = error.kind() {
        if status.raw() == 404 {
          return Ok(None);
        }
      }
      Err(error.into())
    },
  }

  We cannot match directly on the error type in the Err(…) pattern. And, additionally, since we need the status field if it's a response, we can't put it into a conditional clause (as in PATTERN if CONDITIONAL =>), because it's not possible to capture fields there.

  My suggestion would be to make the kind fields on errors public, so they can be matched on.

• Error type enums are in some places called "kinds" and in some places called "types". for example, Error::kind returns an ErrorType, this makes it hard to remember which to use when. I would suggest harmonizing this, and renaming ErrorType to ErrorKind, since this is the most common naming scheme I've seen in other crates.

• The StatusCode type is missing a few desirable features compared to the StatusCode type from the http crate

  http::StatusCode allows comparison with a u16 directly, and has named associated constants for common status codes, e.g. StatusCode::NOT_FOUND, which are convenient. This made working with status codes more challenging, which is exacerbated by the fact that in 0.6, response status codes must be checked in more scenarios.

• In 0.3, requests which might not return a result return Option to indicate that some data wasn't found. In 0.6 this is indicated with a 404 status code.

  On the one hand, it's nice that the 404 status is handled like every other status code. However, in practice, a 404 is often handled differently from other status codes in application client code. It is often handled not as an error that should be propagated via the error handling mechanism, but should instead trigger some special logic. This often makes a request that returns an Option more ergonomic than one that returns a Result::Err on 404.

  Additionally, some Discord API calls return a 404 and some don't. For example, /users/@me never returns a 404, whereas /users/ID may. In order know when to check for a 404 and when not to, a twilight user must consult the Discord API docs. It may be more desirable to encode this in the response type, so that the twilight types document this, and it isn't necessary to fall back on twilight or discord documentation.

• Source errors can now only be obtained by calling Error::source, which returns a Option<&(dyn Error)>, which makes it hard to get more information on the source error. In our code, when the cluster doesn't start, we handle rate-limiting errors specially. If the cluster didn't start due to being rate-limited, we wait and try again, whereas for other errors we return the error upwards.

  This looks something like this:

  match result {
    Ok(cluster) => return Ok(cluster),
    Err(error) => {
      if let Some(http_error) = error.source().unwrap().downcast_ref::<HttpError>() {
        if let twilight_http::error::ErrorType::Response { body, status, .. } =
          http_error.kind()
        {
          if status.raw() == 429 {
            // handle rate limiting error
          }
        }
      }
  
      return Err(error);
    }
  }

  Although this compiles, I'm not sure if this code is correct, becuase I don't know if HttpError is the underlying source error. (Incidentally, this match is made more verbose because the kind and status can't be matched on directly.)

  With 0.3, the code looked like this:

  match result {
    Ok(cluster) => return Ok(cluster),
    Err(ClusterStartError::RetrievingGatewayInfo {
      source: HttpError::Response { body, status, .. },
    }) if status == StatusCode::TOO_MANY_REQUESTS => {
      // handle rate limiting error
    }
    Err(error) => return Err(error),
  }

  My suggestion here would be to consider using snafu. snafu makes it extremely easy to add additional context to a source error, wrapping it in another error. Additionally, this preserves the concrete type of the source error, so that it can be matched on and inspected without calling .source and trying to guess the type with downcast_ref.

Possible Mitigations

I want to underscore that I'm quite grateful that Twilight exists, and, indeed, grateful for its flexibility and modularity, and will continue happily using it regardless. My intention here is just to bring attention to some sharp edges that might be sanded down.

Ideas for mitigating the above issues:

• Use snafu for error types, and keep them as enums. This would allow matching on error variants directly, preserving strongly-typed source errors, and provide additional conveniences to the developers, such as easy conversion from source errors, easy Display impls, and easily adding backtraces, if so desired.

  Failing that, it would be nice if kind fields were public, so they could be matched on. Additionally, it would be nice if kind was used consistently throughout the code.

• Use http::StatusCode, if possible.

• Add a Future implementation to request builders that avoids requiring boilerplate calls to .exec, checking the response and status code, and calling .model or .models.

  The future implementation would essentially look like this:

  // This is the case where the API call may return a 404, and returns
  // a list of models, since this is the most complicated case. If the
  // call cannot return a 404, the return type wouldn't include the
  // `Option`, and if it only returned a single item, the `Vec` would be
  // omitted. If it returned nothing, then the return type would be
  // `Result<(), Error>`
  async fn response_future_impl(self) -> Result<Option<Vec<T>>, Error> {
    let response =  self.exec().await?;
  
    if response.status() == 404 {
      return Ok(None);
    }
  
    if !response.status().is_success() {
      return Err(response.into_error());
    }
  
    let models = response.models()?;
  
    Ok(models)
  }

  This would have two benefits. For what I think would be the majority of users, this would cut down on boilerplate, and let them avoid consulting the discord docs in order to handle status codes correctly. They:

  • Wouldn't need to call .exec
  • Wouldn't need to handle 404s if they wanted to return None
  • Wouldn't need to check the status code
  • Wouldn't need to call .model
  • Wouldn't need to remember to call .model or .models

  I think this would probably be a welcome addition for many users. I think many people like libraries like twilight because it frees them from thinking of protocol-level details such as status codes, single vs multiple responses, and requests that may not return a response, and instead focus on the application semantics of an API, and this would allow them to do just that.

  Additionally, since the .exec and .model path would still be available, users who wanted to do additional checks on the response, for example checking headers or the status code, could do so. And, since the .exec/.model would no longer be the only way to do things, it could be made less user friendly but more useful to users who wanted that additional flexibility. For example, .exec().await might return Ok(Response) in the event of a response, regardless of the status code, so that users wishing to inspect responses can do so without needing to handle Err responses.

[zeylahellyer]

Hi Casey,

I appreciate your detailed feedback about the migration. 0.4 and 0.6 especially included some wide-ranging changes when it comes to upgrading. I'll reply to the things I feel I can reply to in the hopes I can provide insight about intended use cases, the intention of new designs, and what we can improve on.

Pain Points

One thing I do wonder about, is that it's now no longer possible to create multiple event streams from the same cluster that return different event types. This is totally academic, since we don't need it, but I do wonder if it's desirable functionality to have.

When I initially designed the API for {Cluster, Shard}::{some_,}events I had this as a use case in mind. However, I decided that the cost was too high and changed it (#832). Supporting only a single event stream reduced the time to emit a Event::ShardPayload with the raw bytes of a WebSocket message was reduced by nearly 80% and emitting a deserialized event was reduced by nearly 60%. The benchmark results and code can be found in the PR. To create a wrapping Stream over the events stream and emitting to a new MPMC channel - like the Flume crate or Tokio's - shouldn't be too difficult. If you need any assistance here feel free to reach out.

Instead of being able to call .await on a request builder, we now have to call .exec. This isn't so bad, since it's a compile-time error to try to call .await, so it isn't possible to forget to do it. However, it does make the code more verbose. Would it be possible to provide Future implementations for requests that simply forward to .exec?

This was kind of a hacky solution that looked cool when I designed it that, honestly, doesn't have much benefit other than being able to save typing 7 characters. The PR itself (#1008) doesn't go into too much detail, but when paired with the other PRs (#1009, #1010) it makes a bit more sense.

Prior to these PRs you had to give the requests owned data. They could not (reasonably) take references due to lifetime requirements and self referential lifetimes during request execution initialization. We had to clone provided data since you can poll a Future to completion (call .await on it) more than once. We could have just had an Option for all the data and then panicked if someone tried to double-await a request, but that solution gets very messy. Because the exec method on request builders consumes the builder and provides a ResponseFuture<T> we no longer have to deal with all that. Paired with #1009 we can now also take references for most of our arguments without significant API changes. As a bonus, it's now possible to know when a request is in flight due to the dedicated ResponseFuture<T> type and with #1010 we get a lot of functions const. Right now there's not much benefit since you need a constructed Client to even build them, but one day you might even be able to initialize and validate most requests in const contexts.

Instead of returning the model that was requested, requests now return a Response, and it's necessary to call .model on the response to get the model. This isn't a huge problem, since it's not possible to forget, but it does make the code more verbose. This is particularly pronounced in our test code, where we use .unwrap instead of ?:

The intent here is performance, similar to that of the change with exec. Often when you create a message you don't need the resultant message, so there's no reason to spend the CPU time and physical time chunking the response body and deserializing it. The new Response<T> type also allows you to access the response headers and the status code -- something that couldn't be accessed prior. One day I would like to extend this to supporting the operation of HEADing endpoints.

We cannot match directly on the error type in the Err(…) pattern. And, additionally, since we need the status field if it's a response, we can't put it into a conditional clause (as in PATTERN if CONDITIONAL =>), because it's not possible to capture fields there.

My suggestion would be to make the kind fields on errors public, so they can be matched on.

The reason for this change was that we hid the source error behind a struct because we didn't want to expose dependencies, which locks us into them. We exposed types from the HTTP client and WebSocket clients, causing us to not being able to upgrade from say 0.12 to 0.13. I'm not sure if I'm enthusiastic about it since this is an idiomatic design.

Error type enums are in some places called "kinds" and in some places called "types". for example, Error::kind returns an ErrorType, this makes it hard to remember which to use when. I would suggest harmonizing this, and renaming ErrorType to ErrorKind, since this is the most common naming scheme I've seen in other crates.

I would be in favor of unifying these naming schemes. Could you list some examples of other crates so we could create an issue justifying it? I'm having a hard time finding any right now, since most crates just expose their dependencies' errors.

The StatusCode type is missing a few desirable features compared to the StatusCode type from the http crate

http::StatusCode allows comparison with a u16 directly, and has named associated constants for common status codes, e.g. StatusCode::NOT_FOUND, which are convenient. This made working with status codes more challenging, which is exacerbated by the fact that in 0.6, response status codes must be checked in more scenarios.

Good idea. I've opened an issue: #1114

In 0.3, requests which might not return a result return Option to indicate that some data wasn't found. In 0.6 this is indicated with a 404 status code.
On the one hand, it's nice that the 404 status is handled like every other status code. However, in practice, a 404 is often handled differently from other status codes in application client code. It is often handled not as an error that should be propagated via the error handling mechanism, but should instead trigger some special logic. This often makes a request that returns an Option more ergonomic than one that returns a Result::Err on 404.
Additionally, some Discord API calls return a 404 and some don't. For example, /users/@me never returns a 404, whereas /users/ID may. In order know when to check for a 404 and when not to, a twilight user must consult the Discord API docs. It may be more desirable to encode this in the response type, so that the twilight types document this, and it isn't necessary to fall back on twilight or discord documentation.

I'm not sure how I feel about this. I only really did it in the first place since we didn't have this Response<T> abstraction. I'd like to hear from others. My gut feeling is that the API of this would be more difficult to understand than would save. If you often skip around this abstraction I would create a wrapping implementation, such as a function or Future type.

My suggestion here would be to consider using snafu. snafu makes it extremely easy to add additional context to a source error, wrapping it in another error. Additionally, this preserves the concrete type of the source error, so that it can be matched on and inspected without calling .source and trying to guess the type with downcast_ref.

The problem with this is that it would expose the dependency via the public API, correct? We can't really do this. I would like it if it was easier to handle this but I don't feel Rust's Error implementation is quite there yet.

Possible Mitigations

Use snafu for error types, and keep them as enums. This would allow matching on error variants directly, preserving strongly-typed source errors, and provide additional conveniences to the developers, such as easy conversion from source errors, easy Display impls, and easily adding backtraces, if so desired.

Use http::StatusCode, if possible.

See above about exposing dependencies. The simplified explanation again is that exposing dependencies would require us doing a new major version if we want to update one. We want to be able to aim for a 1.0, and doing this would make us either begin to use consistently outdated dependencies or churn through otherwise empty major versions.

I think this would probably be a welcome addition for many users. I think many people like libraries like twilight because it frees them from thinking of protocol-level details such as status codes, single vs multiple responses, and requests that may not return a response, and instead focus on the application semantics of an API, and this would allow them to do just that.

Twilight doesn't actually have the same goal as most other Discord libraries. The goal of Twilight is to expose protocol level details and not define implementations much higher than that. Other libraries tend to abstract things away and make things easier to use for the 90% of people. This is fine, most people either aren't experienced programmers and make a bot for a fun project for their server or just want to get stuff done. However, Twilight aims to cater to the 10% that do need low level access to things for either design reasons, scalability, or simple preference. Using Twilight will be more verbose than alternatives, but you can bet that it'll be faster to use and if you want to do something with it then you can do it.

I hope my reply has helped and thanks again for your feedback. I may not come across as very willing to make changes purely for the intent of ergonomics, and the simple fact is that when it comes to this library I'm not for the reasons just up above. I have created #1114 based on your feedback and would like to open another issue for the ErrorType/ErrorKind naming. I would also like feedback from others about embedding whether a response is expected to be optional in the Response<T> type itself, but I don't expect to hear much feedback considering GitHub discussions aren't visible (it doesn't make it clear that there are any open discussions because there's no badge number in the tab). I will give this a bump in the Discord server in the event that ends up being the case.

[casey]

Hi Zeyla,

Thanks a bunch for the context behind the changes, that make the big picture clearer.

When I initially designed the API for {Cluster, Shard}::{some_,}events I had this as a use case in mind. However, I decided that the cost was too high and changed it (#832). Supporting only a single event stream reduced the time to emit a Event::ShardPayload with the raw bytes of a WebSocket message was reduced by nearly 80% and emitting a deserialized event was reduced by nearly 60%. The benchmark results and code can be found in the PR. To create a wrapping Stream over the events stream and emitting to a new MPMC channel - like the Flume crate or Tokio's - shouldn't be too difficult. If you need any assistance here feel free to reach out.

Ahhh, gotcha. Needing to thread through the Events iterator and being unable to filter in multiple ways wasn't too big of an issue, so if it's a performance win then it's definitely worth it.

Prior to these PRs you had to give the requests owned data. They could not (reasonably) take references due to lifetime requirements and self referential lifetimes during request execution initialization. We had to clone provided data since you can poll a Future to completion (call .await on it) more than once. We could have just had an Option for all the data and then panicked if someone tried to double-await a request, but that solution gets very messy. Because the exec method on request builders consumes the builder and provides a ResponseFuture we no longer have to deal with all that. Paired with #1009 we can now also take references for most of our arguments without significant API changes. As a bonus, it's now possible to know when a request is in flight due to the dedicated ResponseFuture type and with #1010 we get a lot of functions const. Right now there's not much benefit since you need a constructed Client to even build them, but one day you might even be able to initialize and validate most requests in const contexts.

Gotcha, that makes sense. There's a trait called IntoFuture, with a method into_future, which is analogous to IntoIterator, which, once landed, will be called when you do .await. I'm not fully aware of the details, but IntoFuture::into_future takes self by value, then when this lands it could take the place of exec, and allow moving all the data from self into the future without an explicit call to .exec.

The intent here is performance, similar to that of the change with exec. Often when you create a message you don't need the resultant message, so there's no reason to spend the CPU time and physical time chunking the response body and deserializing it. The new Response type also allows you to access the response headers and the status code -- something that couldn't be accessed prior. One day I would like to extend this to supporting the operation of HEADing endpoints.

What would you think of a shortcut method on request builders? For example, a method called get, which does what my example future implementation above would do:

async fn get(self) -> Result<Option<Vec<T>>, Error> {
  let response =  self.exec().await?;

  if response.status() == 404 {
    return Ok(None);
  }

  if !response.status().is_success() {
    return Err(response.into_error());
  }

  let models = response.models()?;

  Ok(models)
}

This would be super convenient for consumers like us, who don't need to look at status codes or response headers, and always want to deserialize and access the models. And, it would preserve the ability to use exec to drop down to a lower level. Also, you mentioned that in the future you might want to require checking a status code with exec, before calling model or models. This would make that change easier to make, since users now have two options, and it can be clear that if you want to handle status codes, you call exec, but if you don't, you can call get.

The reason for this change was that we hid the source error behind a struct because we didn't want to expose dependencies, which locks us into them. We exposed types from the HTTP client and WebSocket clients, causing us to not being able to upgrade from say 0.12 to 0.13. I'm not sure if I'm enthusiastic about it since this is an idiomatic design.

There are a few issues here, one is that since kind is behind a method, it's harder to match on. If the Error types stay structs, then the kind field could be public, since it's unlikely to change:

pub struct Error {
  // public, can be matched on and captured
  pub kind: ErrorKind,
  // private source error
  source: Box<dyn std::error::Error>,
}

I believe the only reason io::Error doesn't have a public kind field is because the internal representation is opaque and platform dependent, and not that it's necessarily idiomatic to hide it:

// from std::io
pub struct Error {
  repr: Repr,
}

Regarding exposing dependencies, is the issue that you have to bump versions when the dependency changes its version, or if you want to switch dependencies? I personally would prefer to have more breaking changes (but with easy migrations), but be able to match on concrete error types, rather than fewer breaking changes but not be able to match on concrete error types, but that's a bit subjective.

I would be in favor of unifying these naming schemes. Could you list some examples of other crates so we could create an issue justifying it? I'm having a hard time finding any right now, since most crates just expose their dependencies' errors.

One good example is io::Error itself, which has io::Error::kind, and io::ErrorKind. Some other examples are nom, bincode, and clap.

The problem with this is that it would expose the dependency via the public API, correct? We can't really do this. I would like it if it was easier to handle this but I don't feel Rust's Error implementation is quite there yet.

I believe that errors that errors that use snafu don't expose anything publicly from snafu, so snafu isn't a visible dependency. Snafu only helps with generating debug implementations, and with making it easy to convert from a source error to the crate's error type.

Use http::StatusCode, if possible.

See above about exposing dependencies. The simplified explanation again is that exposing dependencies would require us doing a new major version if we want to update one. We want to be able to aim for a 1.0, and doing this would make us either begin to use consistently outdated dependencies or churn through otherwise empty major versions.

Sorry, I commented on the issue you created before I saw this. I personally don't see this as a huge downside. I don't mind a library going through major version bumps, especially if it means that they provide a more featureful API. I do understand the hesitation though. The idea of having to go from 1.0 to 2.0 because a version changed is, not great. Semver should have an additional major version W.X.Y.Z, where bumping W or X is a major version bump, so you can keep W at whatever you want it to be, like, 1.0, and bump X when you don't wan to bump the first version number.

Twilight doesn't actually have the same goal as most other Discord libraries. The goal of Twilight is to expose protocol level details and not define implementations much higher than that

I definitely appreciate that, and in fact it's the reason that I chose Twilight over alternatives. I wanted to be sure that if I needed to drop into lower-level details, or swap something out, I would be able to.

That being said, I think that ergonomics could be without having to make compromises. For example by adding a get method to response builders, that would do the convenient thing, while still making exec available.

One other thing is the .model vs .models, I was wondering if there was some context on why they're different?

[zeylahellyer]

There's a trait called IntoFuture, with a method into_future, which is analogous to IntoIterator, which, once landed, will be called when you do .await. I'm not fully aware of the details, but IntoFuture::into_future takes self by value, then when this lands it could take the place of exec, and allow moving all the data from self into the future without an explicit call to .exec.

One day :)

What would you think of a shortcut method on request builders? For example, a method called get, which does what my example future implementation above would do:

Some sort of trait for requests to do things like this may be useful, but maybe that's something that should go in userland under some sort of "utilities" crate. I generally don't like having ergonomics like this in crates because there's plenty of opinionated ways of how you could do it and combine them. This could be worthy of a Discussion topic of its own.

There are a few issues here, one is that since kind is behind a method, it's harder to match on. If the Error types stay structs, then the kind field could be public, since it's unlikely to change:

I'm not going to reply to this at the moment since it would take more time to think about and reply to than I want to spend right now, but I'm not ignoring it and will return to it later.

One other thing is the .model vs .models, I was wondering if there was some context on why they're different?

The goal was to produce code that's self-describing, but for sure model should work in contexts where models does as well. I've opened a PR to address this: #1123

[casey]

Some sort of trait for requests to do things like this may be useful, but maybe that's something that should go in userland under some sort of "utilities" crate. I generally don't like having ergonomics like this in crates because there's plenty of opinionated ways of how you could do it and combine them. This could be worthy of a Discussion topic of its own.

I might roll my own, and when I get more experience with it, open a PR or discussion topic, so I can make a concrete proposal, either for inclusion in the main library, or in a helper crate.

I'm not going to reply to this at the moment since it would take more time to think about and reply to than I want to spend right now, but I'm not ignoring it and will return to it later.

Sounds good, no rush!

The goal was to produce code that's self-describing, but for sure model should work in contexts where models does as well. I've opened a PR to address this: #1123

Awesome, thank you!