Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce that all implementations of the VssHeaderProvider are Send+Sync. #35

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Enforce that all implementations of the VssHeaderProvider are Send+Sync. #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
727c344
Pin tokio-util to 0.7.11 to fix msrv build
G8XSU Sep 7, 2024
3a6d13b
Enforce that all implementations of the VssHeaderProvider are Send+Sync.
G8XSU Sep 7, 2024
4fe2b61
Patch release 0.3.1.
G8XSU Sep 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ jobs:
cargo update -p proptest --precise "1.2.0" --verbose # proptest 1.3.0 requires rustc 1.64.0
cargo update -p regex --precise "1.9.6" --verbose # regex 1.10.0 requires rustc 1.65.0
cargo update -p tokio --precise "1.38.1" --verbose # tokio v1.39.0 requires rustc 1.70 or newer
cargo update -p tokio-util --precise "0.7.11" --verbose # tokio-util v0.7.12 requires rustc 1.70 or newer
- name: Build on Rust ${{ matrix.toolchain }}
run: cargo build --verbose --color always
- name: Check formatting
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "vss-client"
version = "0.3.0"
version = "0.3.1"
rust-version = "1.63.0"
license = "MIT OR Apache-2.0"
edition = "2021"
Expand Down
2 changes: 1 addition & 1 deletion src/headers/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub use lnurl_auth_jwt::LnurlAuthToJwtProvider;

/// Defines a trait around how headers are provided for each VSS request.
#[async_trait]
pub trait VssHeaderProvider {
pub trait VssHeaderProvider: Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary or could we do a cast like as &(dyn VssHeaderProvider + Send + Sync) or similar at the site were we use it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm.. i thought it was nice to have to make it easier to use header providers by everyone, instead of having some headerproviders which are not send+sync.

as &(dyn VssHeaderProvider + Send + Sync)

I wasn't able to fix it just using that maybe i am missing something.
In VssStore in ldk-node we aren't using any headerprovider as of now, so VssClient is created with FixedHeaders as headerProvider.
And VssClient contains Arc<dyn VssHeaderProvider> as its member, somehow compiler isn't able to determine it as send + sync unless I mark the member explicitly as headers: Arc<dyn VssHeaderProvider + Send + Sync>

I am not sure if there is much difference in enforcing all impls as Send + Sync or marking it in vssClient as

pub struct VssClient<R>
where
	R: RetryPolicy<E = VssError>,
{
	base_url: String,
	client: Client,
	retry_policy: R,
	header_provider: Arc<dyn VssHeaderProvider + Send + Sync>,
}

Let me know if you are suggesting something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. i thought it was nice to have to make it easier to use header providers by everyone, instead of having some headerproviders which are not send+sync.

Well, usually you'd avoid adding these markers as the compiler is mostly able to figure them out on its own, and there might be instances where someone wants to use VssHeaderProvider but doesn't require it to be Send+Sync.

I wasn't able to fix it just using that maybe i am missing something. In VssStore in ldk-node we aren't using any headerprovider as of now, so VssClient is created with FixedHeaders as headerProvider. And VssClient contains Arc<dyn VssHeaderProvider> as its member, somehow compiler isn't able to determine it as send + sync unless I mark the member explicitly as headers: Arc<dyn VssHeaderProvider + Send + Sync>

I am not sure if there is much difference in enforcing all impls as Send + Sync or marking it in vssClient as

pub struct VssClient<R>
where
	R: RetryPolicy<E = VssError>,
{
	base_url: String,
	client: Client,
	retry_policy: R,
	header_provider: Arc<dyn VssHeaderProvider + Send + Sync>,
}

Let me know if you are suggesting something else.

There is a difference, for one, as mentioned above, there might be instances where you don't need it to be Send+Sync and requiring it might lead to unnecessary complication and of course in terms of trait inheritance it's not super logical as generally VssHeaderProvider and threading-related traits are really more or less unrelated concepts.

I think usually it's preferred to just include the explicit + Send + Sync where needed, but there also might be exceptions to it, and also no big deal if you prefer otherwise. However you decide, happy to move forward with this PR, i.e., def. shouldn't be a blocker here.

Copy link
Collaborator Author

@G8XSU G8XSU Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am ok with adding Send+Sync at place of use but as described above, couldn't make it work other than adding it at VssClient header_provider member.
We expect VssClient to be Send+Sync and VssHeaderProvider is a member in it, hence it should be Send+Sync.

I can add

pub struct VssClient<R>
where
	R: RetryPolicy<E = VssError>,
{
	base_url: String,
	client: Client,
	retry_policy: R,
	header_provider: Arc<dyn VssHeaderProvider + Send + Sync>,
}

if that makes sense.

/// Returns the HTTP headers to be used for a VSS request.
/// This method is called on each request, and should likely perform some form of caching.
///
Expand Down
Loading