LemmyNet
/
lemmy
3
2
Fork 3
Code Pull Requests 1 Releases Wiki Activity

Implement context #86

Merged
dessalines merged 1 commits from implement-context into main 2020-08-18 13:43:54 +00:00
Conversation 7 Commits 1 Files Changed 34 +1535 -1607
nutomic commented 2020-08-14 14:20:09 +00:00
Owner
Copy Link

I'm trying to implement a Context struct, so we dont have to pass around DbPool, ChatServer, Client, and soon ActivitySender all the time. This isnt going to change any logic or behaviour, just make parameter lists shorter. Its essentially #1082, but without the request_counter (that should go somewhere else as the context should be stateless).

At the moment its failing with this error, any idea how to fix it?

Checking lemmy_server v0.0.1 (/home/felix/workspace/lemmy/server)
error[E0277]: `std::rc::Rc<awc::ClientConfig>` cannot be sent between threads safely
--> src/main.rs:73:3
|
73  |     HttpServer::new(move || {
|  ___^^^^^^^^^^^^^^^_-
| |   |
| |   `std::rc::Rc<awc::ClientConfig>` cannot be sent between threads safely
74  | |     let settings = Settings::get();
75  | |     let rate_limiter = rate_limiter.clone();
76  | |     App::new()
...   |
99  | |       ))
100 | |   })
| |___- within this `[closure@src/main.rs:73:19: 100:4 rate_limiter:lemmy_server::rate_limit::RateLimit, context:lemmy_server::LemmyContext, pool:embedded_migrations::diesel::r2d2::Pool<embedded_migrations::diesel::r2d2::ConnectionManager<embedded_migrations::diesel::PgConnection>>, chat_server:actix::address::Addr<lemmy_server::websocket::server::ChatServer>]`
|
= help: within `[closure@src/main.rs:73:19: 100:4 rate_limiter:lemmy_server::rate_limit::RateLimit, context:lemmy_server::LemmyContext, pool:embedded_migrations::diesel::r2d2::Pool<embedded_migrations::diesel::r2d2::ConnectionManager<embedded_migrations::diesel::PgConnection>>, chat_server:actix::address::Addr<lemmy_server::websocket::server::ChatServer>]`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<awc::ClientConfig>`
= note: required because it appears within the type `awc::Client`
= note: required because it appears within the type `lemmy_server::LemmyContext`
= note: required because it appears within the type `[closure@src/main.rs:73:19: 100:4 rate_limiter:lemmy_server::rate_limit::RateLimit, context:lemmy_server::LemmyContext, pool:embedded_migrations::diesel::r2d2::Pool<embedded_migrations::diesel::r2d2::ConnectionManager<embedded_migrations::diesel::PgConnection>>, chat_server:actix::address::Addr<lemmy_server::websocket::server::ChatServer>]`
= note: required by `actix_web::server::HttpServer::<F, I, S, B>::new`
I'm trying to implement a `Context` struct, so we dont have to pass around DbPool, ChatServer, Client, and soon ActivitySender all the time. This isnt going to change any logic or behaviour, just make parameter lists shorter. Its essentially [#1082](https://github.com/LemmyNet/lemmy/issues/1082), but without the request_counter (that should go somewhere else as the context should be stateless). At the moment its failing with this error, any idea how to fix it? ``` Checking lemmy_server v0.0.1 (/home/felix/workspace/lemmy/server) error[E0277]: `std::rc::Rc<awc::ClientConfig>` cannot be sent between threads safely --> src/main.rs:73:3 | 73 | HttpServer::new(move || { | ___^^^^^^^^^^^^^^^_- | | | | | `std::rc::Rc<awc::ClientConfig>` cannot be sent between threads safely 74 | | let settings = Settings::get(); 75 | | let rate_limiter = rate_limiter.clone(); 76 | | App::new() ... | 99 | | )) 100 | | }) | |___- within this `[closure@src/main.rs:73:19: 100:4 rate_limiter:lemmy_server::rate_limit::RateLimit, context:lemmy_server::LemmyContext, pool:embedded_migrations::diesel::r2d2::Pool<embedded_migrations::diesel::r2d2::ConnectionManager<embedded_migrations::diesel::PgConnection>>, chat_server:actix::address::Addr<lemmy_server::websocket::server::ChatServer>]` | = help: within `[closure@src/main.rs:73:19: 100:4 rate_limiter:lemmy_server::rate_limit::RateLimit, context:lemmy_server::LemmyContext, pool:embedded_migrations::diesel::r2d2::Pool<embedded_migrations::diesel::r2d2::ConnectionManager<embedded_migrations::diesel::PgConnection>>, chat_server:actix::address::Addr<lemmy_server::websocket::server::ChatServer>]`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<awc::ClientConfig>` = note: required because it appears within the type `awc::Client` = note: required because it appears within the type `lemmy_server::LemmyContext` = note: required because it appears within the type `[closure@src/main.rs:73:19: 100:4 rate_limiter:lemmy_server::rate_limit::RateLimit, context:lemmy_server::LemmyContext, pool:embedded_migrations::diesel::r2d2::Pool<embedded_migrations::diesel::r2d2::ConnectionManager<embedded_migrations::diesel::PgConnection>>, chat_server:actix::address::Addr<lemmy_server::websocket::server::ChatServer>]` = note: required by `actix_web::server::HttpServer::<F, I, S, B>::new` ```
nutomic commented 2020-08-14 18:53:30 +00:00
Author
Owner
Copy Link

Okay this is almost working, the only thing left is figuring out a proper lifetime for the return types on context functions. Or we could return by value instead, but returning by reference seems better (maybe I'm wrong though).

error[E0495]: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
--> src/lib.rs:85:42
|
85 |   pub fn pool<'a>(&self) -> &'a DbPool { &self.pool }
|                                          ^^^^^^^^^^
|
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the method body at 85:3...
--> src/lib.rs:85:3
|
85 |   pub fn pool<'a>(&self) -> &'a DbPool { &self.pool }
|   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...so that reference does not outlive borrowed content
--> src/lib.rs:85:42
|
85 |   pub fn pool<'a>(&self) -> &'a DbPool { &self.pool }
|                                          ^^^^^^^^^^
note: but, the lifetime must be valid for the lifetime `'a` as defined on the method body at 85:15...
--> src/lib.rs:85:15
|
85 |   pub fn pool<'a>(&self) -> &'a DbPool { &self.pool }
|               ^^
note: ...so that reference does not outlive borrowed content
--> src/lib.rs:85:42
|
85 |   pub fn pool<'a>(&self) -> &'a DbPool { &self.pool }
|                                          ^^^^^^^^^^
Okay this is almost working, the only thing left is figuring out a proper lifetime for the return types on context functions. Or we could return by value instead, but returning by reference seems better (maybe I'm wrong though). ``` error[E0495]: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements --> src/lib.rs:85:42 | 85 | pub fn pool<'a>(&self) -> &'a DbPool { &self.pool } | ^^^^^^^^^^ | note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the method body at 85:3... --> src/lib.rs:85:3 | 85 | pub fn pool<'a>(&self) -> &'a DbPool { &self.pool } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: ...so that reference does not outlive borrowed content --> src/lib.rs:85:42 | 85 | pub fn pool<'a>(&self) -> &'a DbPool { &self.pool } | ^^^^^^^^^^ note: but, the lifetime must be valid for the lifetime `'a` as defined on the method body at 85:15... --> src/lib.rs:85:15 | 85 | pub fn pool<'a>(&self) -> &'a DbPool { &self.pool } | ^^ note: ...so that reference does not outlive borrowed content --> src/lib.rs:85:42 | 85 | pub fn pool<'a>(&self) -> &'a DbPool { &self.pool } | ^^^^^^^^^^ ```
nutomic changed title from Attempt to implement context to Implement context 2020-08-17 14:08:40 +00:00
nutomic commented 2020-08-17 14:09:15 +00:00
Author
Owner
Copy Link

Updated, I finished replacing everything. Now I need to figure out why all the federation tests are failing.

Updated, I finished replacing everything. Now I need to figure out why all the federation tests are failing.
dessalines commented 2020-08-17 14:38:43 +00:00
Owner
Copy Link

Some of the errors I'm seeing:

lemmy-beta_1      | [2020-08-17T14:36:21Z DEBUG actix_web::data] Failed to construct App-level Data extractor. Request path: "/c/main
"
lemmy-beta_1      | [2020-08-17T14:36:21Z ERROR actix_http::response] Internal Server Error: "App data is not configured, to configur
e use App::data()"
lemmy-beta_1      | [2020-08-17T14:36:21Z INFO  actix_web::middleware::logger] 172.21.0.8:46800 "GET /c/main HTTP/1.1" 500 56 "-" "-"
 0.005050
lemmy-alpha_1     | [2020-08-17T14:36:21Z DEBUG lemmy_server::apub::fetcher] Receive error, Content type error
lemmy-alpha_1     | [2020-08-17T14:36:21Z DEBUG lemmy_server::api::site] Failed to resolve search query as activitypub ID: Error rece
iving response, Content type error
Some of the errors I'm seeing: ``` lemmy-beta_1 | [2020-08-17T14:36:21Z DEBUG actix_web::data] Failed to construct App-level Data extractor. Request path: "/c/main " lemmy-beta_1 | [2020-08-17T14:36:21Z ERROR actix_http::response] Internal Server Error: "App data is not configured, to configur e use App::data()" lemmy-beta_1 | [2020-08-17T14:36:21Z INFO actix_web::middleware::logger] 172.21.0.8:46800 "GET /c/main HTTP/1.1" 500 56 "-" "-" 0.005050 lemmy-alpha_1 | [2020-08-17T14:36:21Z DEBUG lemmy_server::apub::fetcher] Receive error, Content type error lemmy-alpha_1 | [2020-08-17T14:36:21Z DEBUG lemmy_server::api::site] Failed to resolve search query as activitypub ID: Error rece iving response, Content type error ```
dessalines commented 2020-08-17 14:59:44 +00:00
Owner
Copy Link

May have found the issue: run grep -r "web::Data" server/src/

There's a lot of web::Data refs that need to change to context. Its annoying that actix doesn't warn you about these.

May have found the issue: run `grep -r "web::Data" server/src/` There's a lot of `web::Data` refs that need to change to context. Its annoying that actix doesn't warn you about these.
nutomic commented 2020-08-17 17:26:54 +00:00
Author
Owner
Copy Link

Ah I didnt even see your comments. Yep that was the problem, I went through our routes and replaced all the web::Data. Might be good to double check cause as you said, the compiler doesnt warn about it.

Also this was a lot of mindless work replacing everything, hopefully we dont have to do that again any time soon.

Edit: Federation tests are passing now.

Ah I didnt even see your comments. Yep that was the problem, I went through our routes and replaced all the web::Data. Might be good to double check cause as you said, the compiler doesnt warn about it. Also this was a lot of mindless work replacing everything, hopefully we dont have to do that again any time soon. Edit: Federation tests are passing now.
nutomic commented 2020-08-17 18:30:56 +00:00
Author
Owner
Copy Link

Rebased. Btw I had a failure in the test Sticky a post, but it passed fine when I ran the tests again.

● Sticky a post

TypeError: Cannot read property 'community_local' of undefined

95 |   let searchBeta2 = await searchPost(beta, postRes.post);
96 |   let betaPost2 = searchBeta2.posts[0];
>  97 |   expect(betaPost2.community_local).toBe(true);
|                    ^
98 |   expect(betaPost2.creator_local).toBe(false);
99 |   expect(betaPost2.stickied).toBe(false);
100 | });

at src/api_tests/post.spec.ts:97:20
at fulfilled (node_modules/tslib/tslib.js:112:62)
Rebased. Btw I had a failure in the test `Sticky a post`, but it passed fine when I ran the tests again. ``` ● Sticky a post TypeError: Cannot read property 'community_local' of undefined 95 | let searchBeta2 = await searchPost(beta, postRes.post); 96 | let betaPost2 = searchBeta2.posts[0]; > 97 | expect(betaPost2.community_local).toBe(true); | ^ 98 | expect(betaPost2.creator_local).toBe(false); 99 | expect(betaPost2.stickied).toBe(false); 100 | }); at src/api_tests/post.spec.ts:97:20 at fulfilled (node_modules/tslib/tslib.js:112:62) ```
dessalines commented 2020-08-18 13:43:40 +00:00
Owner
Copy Link

Looks good, and all the tests passed. The only other web:Data< references are in images.rs, but you're creating another client there, so its good.

Looks good, and all the tests passed. The only other `web:Data<` references are in `images.rs`, but you're creating another client there, so its good.
dessalines merged commit 14f2d190e5 into main 2020-08-18 13:43:54 +00:00
nutomic deleted branch implement-context 2020-08-18 13:46:31 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: LemmyNet/lemmy#86
No description provided.
Delete Branch "implement-context"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?