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.

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

Consider using std::sync instead of parking_lot #438

Closed
matklad opened this issue Jun 8, 2022 · 2 comments · Fixed by #473
Closed

Consider using std::sync instead of parking_lot #438

matklad opened this issue Jun 8, 2022 · 2 comments · Fixed by #473

Comments

@matklad
Copy link
Contributor

matklad commented Jun 8, 2022

paperclip today uses parking_lot. parking_lot provides faster, smaller mutexes, at the cost of adding a somewhat churn-prone dependency. It's not uncommon for downstream projects to end up with a couple of different versions of parking_lot in the Cargo.lock.

Naively, I would assume that mutext perfomance should not matter for the thing that paperclip is doing, and that sticking with std mutexes would make more sense, especially in the light of rust-lang/rust#95035.

@tiagolobocastro
Copy link
Collaborator

I believe you are right, mutex performance should not be critical here.
One snag might be the usage of parking_lot mutex on the autogenerated client code via CLI, as that is a public api.

As an aside, it's interesting how it's being used there. At a first glance, not sure why we need the response wrapped in a mutex.

    pub enum ApiError<R: Debug + Send + 'static> {
        #[error("API request failed for path: {} (code: {})", _0, _1)]
        Failure(String, http::status::StatusCode, Mutex<R>),
        #[error("Unsupported media type in response: {}", _0)]
        UnsupportedMediaType(String, Mutex<R>),
        #[error("An error has occurred while performing the API request: {}", _0)]
        Reqwest(reqwest::Error),.......
    }

@matklad
Copy link
Contributor Author

matklad commented Jun 8, 2022

One snag might be the usage of parking_lot mutex on the autogenerated client code via CLI, as that is a public api.

Note that as pl released a new major version, you'd have to break that anyway to upgrade pl. So, in principle that's another argument for switching to std.

But yeah, that's going to be a semver-breaking change either way, and yeah, it might make sense to maybe remove mutexes from public API altogether. That ApiError looks like something very prone to breakage.

tiagolobocastro added a commit that referenced this issue Oct 16, 2022
As pointed out in #438 the mutex performance between parking_lot should not adversely affect
paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
tiagolobocastro added a commit that referenced this issue Oct 16, 2022
As pointed out in #438 the mutex performance between parking_lot should not adversely affect
paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Resolves: #438

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
tiagolobocastro added a commit that referenced this issue Oct 16, 2022
As pointed out in #438 the mutex performance between parking_lot should not adversely affect
paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Resolves: #438

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
tiagolobocastro added a commit that referenced this issue Oct 16, 2022
As pointed out in #438 the mutex performance between parking_lot should not adversely affect
paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Resolves: #438

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
tiagolobocastro added a commit that referenced this issue Oct 18, 2022
As pointed out in #438 the mutex performance between parking_lot should not adversely affect
paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Resolves: #438

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
tiagolobocastro added a commit that referenced this issue Oct 18, 2022
As pointed out in 438 the mutex performance between parking_lot should not adversely affect
paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Resolves: #438

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
tiagolobocastro added a commit that referenced this issue Oct 18, 2022
refactor!: switch from parking_lot to std::sync

As pointed out in #438 the mutex performance between parking_lot should not adversely affect paperclip.
Also remove the mutexes altogether from the ApiError.

Unfortunately this is indeed a breaking change!

Resolves: #438

Signed-off-by: Tiago Castro tiagolobocastro@gmail.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants