Error returned by `Blocking::sample` is not marked `Send + Sync`

[avandesa]

Because of this, wrap_eyre from the color_eyre crate doesn't work.

Blocking::sample returns Result<Sample, Box<dyn Error>>, since the error it returns is either std::num::TryFromIntError or TelemetryError. All of these error types are Send and Sync, so sample should be able to return Result<Sample, Box<dyn Error + Send + Sync>>.

Digging deeper, in the 'happy path' Blocking::sample returns Header::telemetry, which appears to actually be infallible despite returning Result<Sample, Box<dyn Error>>. I would suggest making this method return Sample, though that would be a breaking change. A non-breaking solution would be to change the return type to Result<Sample, Box<dyn Error + Send + Sync>> or Result<Sample, std::convert::Infallible>>.

Digging even deeper, it doesn't appear that Blocking::sample might not have to return TryFromIntError. This happens when the duration represented in milliseconds exceeds u32::MAX. In my humble opinion, if the user passes in a timeout that's longer than the longest valid timeout for winapi, then it would be appropriate to panic here, assuming that the longest timeout is appropriately documented. That way, you could directly return Result<Sample, TelemetryError>, simplifying the API.

If you want to avoid the panic, then I'd respectfully ask you to consider returning a concrete type

Again, that's a breaking change, so the short-term non-breaking solution could be to just add Send + Sync to the return type.

Thanks!

P.S. The Rust API Guidelines recommend that error types implement Send and Sync.

[avandesa]

Also, I'd be happy to open a PR to make any of these changes, if you'd prefer.

[avandesa]

I just now noticed that Sample::get returns Result<Value, String>, which is an issue because String does not implement std::error::Error. This also causes issues with error handling libraries, as detailed in the API guidelines.

[LeoAdamek]

Thanks for the feedback.

This project was actually my first time using Rust so it doesn't surprise me there are such issues.

Right now the library is not at v1.0 so I'm not opposed to breaking changes with proper notice.

To clarify, your suggestion is to remove the Result from the return of Header::telemetry as the code path does in fact always return an Ok. And to return a more concrete error type, or at least add + Send + Sync to the return types of the blocking reader (in the event of a timeout or Win32 error?)

[avandesa]

your suggestion is to remove the Result from the return of Header::telemetry as the code path does in fact always return an Ok.

Yep! If there's no error to return then might as well remove it from the type signature.

And to return a more concrete error type, or at least add + Send + Sync to the return types of the blocking reader

Yes, Send + Sync where possible. This ensures that it plays nicely with popular error handling libraries and that errors can be passed between threads, if necessary. A concrete error type is better though, since consumers of your API can introspect the error that occured if they need to without downcasting.

Thanks, and sorry for all the typos in my initial post, must've been tired, haha.