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

StatsdClient should implement RefUnwindSafe #77

Closed
Diggsey opened this issue Oct 2, 2018 · 2 comments
Closed

StatsdClient should implement RefUnwindSafe #77

Diggsey opened this issue Oct 2, 2018 · 2 comments

Comments

@Diggsey
Copy link

Diggsey commented Oct 2, 2018

All Sync types should also implement RefUnwindSafe as the former implies the latter.

The StatsdClient currently does not implement RefUnwindSafe because it stores two trait objects:

    sink: Arc<MetricSink + Sync + Send>,
    errors: Arc<Fn(MetricError) -> () + Sync + Send>,

The RefUnwindSafe bound should be added to both of these:

    sink: Arc<MetricSink + Sync + Send + RefUnwindSafe>,
    errors: Arc<Fn(MetricError) -> () + Sync + Send + RefUnwindSafe>,

And then the client will automatically become RefUnwindSafe itself.

@56quarters
Copy link
Owner

Thanks for the issue! I'll start looking into this.

Seems like it may be a little tricky since QueuingMetricSink makes use of a CondVar(1) and a Crossbeam MsQueue(2) which in turn uses UnsafeCell. It'll be interesting to see what comes from the issue you opened in rust-lang/rust#54768

@56quarters
Copy link
Owner

56quarters commented Oct 31, 2018

The two places that prevent the client from being panic safe are both in the QueuingMetricSink.

I've replaced the use of CondVar with an atomic and thread::yield_now() - 36e809d93b64e38ab64ba94ab221a3dc071396a1^...HEAD

The other place that prevents QueuingMetricSink from being panic safe is the use of a crossbeam::MsQueue. I believe it's safe to wrap this in AssertUnwindSafe based on the issue you opened rust-lang/rust#54768. I'm currently in the process of writing a few more tests to increase my comfort with the change.

56quarters added a commit that referenced this issue Nov 2, 2018
Make sure that the StatsdClient is unwind (panic) safe by ensuring
that pointers to sinks and error handlers require the object to be
unwind safe.

Make the QueuingMetricSink unwind safe by not using a CondVar and
Mutex but instead using an AtomicBool to indicate when the worker
is stopped. Additionally, assert that the Crossbeam MsQueue is
unwind safe because it implements Sync.

See rust-lang/rust#54768

Fixes #77
56quarters added a commit that referenced this issue Nov 4, 2018
Make sure that the StatsdClient is unwind (panic) safe by ensuring
that pointers to sinks and error handlers require the object to be
unwind safe.

Make the QueuingMetricSink unwind safe by not using a CondVar and
Mutex but instead using an AtomicBool to indicate when the worker
is stopped. Additionally, assert that the Crossbeam MsQueue is
unwind safe because it implements Sync.

See rust-lang/rust#54768

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

No branches or pull requests

2 participants