Skip to content

feat: expose ConnectionGuard as request extension#1443

Merged
niklasad1 merged 3 commits intoparitytech:masterfrom
dinhani:master
Aug 19, 2024
Merged

feat: expose ConnectionGuard as request extension#1443
niklasad1 merged 3 commits intoparitytech:masterfrom
dinhani:master

Conversation

@dinhani
Copy link
Contributor

@dinhani dinhani commented Aug 16, 2024

Fixes #1438

Since ConnectionGuard keeps the internal information inside an Arc, it should be cheap to clone it for each request.

@dinhani dinhani requested a review from a team as a code owner August 16, 2024 15:37
@niklasad1
Copy link
Contributor

Thanks

Can you just create a simple test for this? Similar to https://github.com/paritytech/jsonrpsee/blob/master/tests/tests/integration_tests.rs#L210-#L225 but you need to add an new handler that actually fetches the ConnGuard from the extension.

request.extensions_mut().insert::<ConnectionId>(conn.conn_id.into());
let req_ext = request.extensions_mut();
req_ext.insert::<ConnectionGuard>(conn_guard.clone());
req_ext.insert::<ConnectionId>(conn.conn_id.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, we could probably merge the ConnectionID and ConnectionGuard as some point to another type e.g. ConnectionDetails or something

}

#[tokio::test]
async fn connection_guard_extension_with_different_ws_clients() {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's sufficient with one client but this does hurt, thanks :)

@dinhani
Copy link
Contributor Author

dinhani commented Aug 16, 2024

@niklasad1
I did not notice code was being tested via JSON-RPC interface.

I tried to put the new test together with the one you pointed, but I think it would become too complex, so I created a new test and renamed the other one to make it clear which extension each one is testing.

@niklasad1
Copy link
Contributor

niklasad1 commented Aug 16, 2024

I did not notice code was being tested via JSON-RPC interface.

I tried to put the new test together with the one you pointed, but I think it would become too complex, so I created a new test and renamed the other one to make it clear which extension each one is testing.

Yeah, you made the right call by creating another test. All good, just run $ cargo +stable fmt --all on your PR which will make the CI happy

@mayconamaro
Copy link

that's nice the PR is already created <3
thank you @dinhani!

@niklasad1 could you suggest another reviewer with write access for this PR?

@niklasad1 niklasad1 requested a review from a team August 16, 2024 19:38
@niklasad1 niklasad1 merged commit 9b66e9c into paritytech:master Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose current connections count as request extension

4 participants