iracing.rs icon indicating copy to clipboard operation
iracing.rs copied to clipboard

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

Open avandesa opened this issue 4 years ago • 4 comments

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 avatar Feb 06 '21 23:02 avandesa