Serenity.await #803
Closed
Serenity.await #803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is technically a breaking change, but I doubt its removal will affect anyone. There's no meaningful usage of an `Arc<T> where T: Framework` without `Mutex` or `RwLock`-based interior-mutability indirection as the framework method requires mutable access of `self` (`&mut self`). This can be hard to achieve with `Arc` because there must be only one owner of the pointer at the time, nullifying its usefulness for shared memory. In summary, this impl is useless.
The 'http::raw' module was initially intended as a home for oneshot request functions. As this has now gone away and been replaced by a full client, the module itself should be renamed to 'client' to match it.
Remove the following exports from the 'client' module: - crate::gateway - crate::http as rest Long, long, long ago, 'http' was called 'rest', and both 'gateway' and 'rest' were located in the 'client' module. These haven't been here since 0.2.x (May of 2017!), so really people should've moved on at this point. Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>
In the 'http' module's 'Http' struct, store the inner reqwest client in an Arc. This doesn't have much use right now for serenity itself, but it does allow users to pass in a reference to their own client when making a client via 'http::Http::new', avoiding the need to have multiple clients. The primary use for this is to ease a slight rework of the 'http::ratelimiting' module. Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>
Rework the ratelimiting structure to avoid an odd hierarchy and to remove the static variable. Ratelimiting was done through a freefloating function named 'perform'. It accepted a reference to the Http client instance and the request to perform. This is backwards in design: the Http client in this design is passing itself to the ratelimiting module, which in turn calls back into the Http passed into it to access route ratelimit information and the global ratelimiter mutex on the Http client. At that point, you may as well put the ratelimiting on the Http client itself. Another poor design choice was to use a 'static mut' for storing the offset. Although this is currently only set once -- after the first response ever is received -- it's noted[1] in the source right now that it may be preferable to mutate it more often in the future. The first issue fixed is the ratelimiting design: the ratelimiter module now has a struct named 'ratelimiter' that owns: - its own global ratelimiting mutex - an Arc<ReqwestClient> that it shares with the Http client - the offset stored as an atomic - a map of routes to their ratelimits - the bot token With this design, ratelimiting information is self-contained. The second issue was the offset. The public interface is the same here, but now on the 'ratelimiter' struct. That is, the function definition still returns an 'option<i64>', despite the fact that it's internally stored as an atomic i64. The value is i64::MAX when no offset has been calculated yet. While not perfect still, there have been some efficiency improvements. The lock for each route is no longer held throughout the duration of each request to that route. This means that 5 message sends in the same channel, assuming a limit of 5, will happen more or less simultaneously if sent all in one go on 5 threads. A minour breaking change is that the 'ratelimit' struct containing the 'limit', 'remaining', and 'reset' for a route are now private structfields, exposed through public getters. This probably doesn't affect any users. Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>
Use `CacheRwLock` in `CacheAndHttp`. Replacing the normal `Arc<RwLock<Cache>>` with the API-friendly `CacheRwLock`. Implement `CacheHttp` for `CacheAndHttp`. Also takes `Arc` and `&T` into account.
After the `v0.6.x`-branch has been deleted, our badge-link no longer pointed to a valid target-branch. This changes the target to the `next`-branch.
Its default value will now be `false`.
This commit actually consists of three changes to make `AttachmentType` significantly more useable and (when done properly) significantly more efficient. - `AttachmentType` variants that used a tuple before now use a struct with named fields, making the code more self-documenting. - `AttachmentType` variants that needed a filename now use `String` instead of `&str`, preventing a `.to_string()` call later on and giving the end library user the responsibility (which in turn also prevents any potential lifetime pitfalls) - `AttachmentType::Bytes` now uses a `std::borrow::Cow` instead of a lifetimed-borrow. When uploading attachments it is not always desirable to pass a reference due to the way people structure their applications, especially as later on when actually sending the attachment the reference is copied by calling `to_vec`. Using a cow annihilates this completely because using `Cow::Owned::into_owned()` is a no-op, thus preventing a potential copy of up to 50MB.
…e cache update timeout (serenity-rs#720)
These were removed in serenity-rs@56fe660 as they duplicated code from `new`, and have therefore been replaced by `new_with_extras`. But the replacement doesn't justify their *whole* removal. Instead, they'll serve as shorthands for using the new method.
…renity-rs#726) The client module has the http module defined as its dependency; it can NOT function without it. This makes the absency of the http feature impossible, and permits this commit to remove all assumptions of its vacancy in the client code.
* Added new audit log values * Remove unused import
|
I aim to merge this pull request soon, we are still checking if the OOM bug - that affects only certain users - is fixed with the recent changes. The final breaking change has been merged, it brings a new cache designed to clone items instead of providing them locked. This includes items' fields, all structs will be without locks now. About the cache redesign: Deadlocks caused by cache locking are a common and hard to debug error, therefore the cache's fields are now private. The single lock around the cache was broken down into multiple locks around each field. Oh and of course: If there is anything missing on the cache, feel free to request it! |
|
I migrated my toy bot to use async as a test, and got an oom this morning. Let me know what I can do to help |
|
Closing this in favour of #905 for continuing the async/await development due to inactivity here. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request rewrites Serenity to be asynchronous.
As highly demanded (#729), Serenity is finally async.
For a long time, it was nothing but a dream, but now it's reality.
There are still a few things following in this PR, but it's ready to be used, see example 5.
Missing bits:
voice-module;voice-examples (6, 10);native_tls_backend;To fix:
Also, collectors (as mentioned in #786) were added too.
You can test it already!