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

Add bounds to AsyncTransport, Ok, and Error #772

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhillyerd
Copy link
Contributor

@jhillyerd jhillyerd commented May 29, 2022

Adding Send + Sync bounds to AsyncTransport and associated types (for #770)

This reduces the amount of bounds that must be specified for a function that is generic over AsyncTransport.

In my case:

pub async fn send_alert<T>(mailer: &T, subject: &str, body: &str) -> anyhow::Result<()>
where
    T: AsyncTransport + Send + Sync,
    <T as AsyncTransport>::Error: 'static + Send + Sync,
    <T as AsyncTransport>::Error: std::error::Error,
{

becomes

pub async fn send_alert<T>(mailer: &T, subject: &str, body: &str) -> anyhow::Result<()>
where
    T: AsyncTransport,
    <T as AsyncTransport>::Error: 'static,
    <T as AsyncTransport>::Error: std::error::Error,
{

I am not certain if adding the 'static and std:error bounds would be reasonable.

@paolobarbolini paolobarbolini linked an issue May 29, 2022 that may be closed by this pull request
@paolobarbolini
Copy link
Member

paolobarbolini commented May 30, 2022

I'm all for adding 'static to Ok, 'static + StdError to Err. I don't think it'd be a problem to also have Send + Sync, but I'd first look into if there is a way of living without ever having to specify them anywhere

@jhillyerd
Copy link
Contributor Author

Just to make sure I understand, you'd like to start with

adding 'static to Ok, 'static + StdError to Err

and revert the Send + Sync change for now?

@paolobarbolini
Copy link
Member

Yes. I wouldn't necessarily revert Send + Sync though, but I'd look into if it's really necessary

@paolobarbolini
Copy link
Member

I'm thinking of going forward with 0.10.0 without this as it's not a major issue and it will buy us more time to think about it

@jhillyerd
Copy link
Contributor Author

I think that's fine, I'm not sure when I'll have time to really dig into this. I've learned a bit more about the expectations around Sync/Send, my current thinking is that forcing folks into Send may be a mistake if they need thread-local resources. Sync still seems like table stakes for an Async API though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add AsyncTransport Send + Sync type bounds?
2 participants