Asynchronous cancellation for ddprof_ffi_ProfileExporterV3_send by ivoanjo · Pull Request #45 · DataDog/libddprof

ivoanjo · 2022-04-04T11:12:22Z

What does this PR do?

Add CancellationToken to ffi, and change exporter to return early from a send if CancellationToken gets marked as cancelled.

Motivation

By default, Ruby (and Java) set a profile export timeout of 30 seconds AKA in the worst case, a call to ddprof_ffi_ProfileExporterV3_send can block for that amount of time.

For Ruby in particular, we want to call ddprof_ffi_ProfileExporterV3_send from a Ruby thread, but there needs to be a way to interrupt a long-running send in case the
application wants to exit (e.g. the user pressed ctrl+c).

Without a way to interrupt a send, the Ruby VM will hang until the timeout is hit.

Additional Notes

I'm marking this PR as WIP since I need some help with the lifetime of the CancellationToken: right now it's getting dropped by the functions that use it, whereas it should only be dropped when ddprof_ffi_CancellationToken_drop manually does it.

(Fixed!)

How to test the change?

I've modified the exporter.cpp to use the cancellation mechanism: if a request takes longer than 5 seconds, it will call cancel and cause send to return earlier than the set timeout.

morrisonlevi

Looks okay, just have a comment/question about the pub struct CancellationToken.

By default, Ruby (and Java) set a profile export timeout of 30 seconds AKA in the worst case, a call to `ddprof_ffi_ProfileExporterV3_send` can block for that amount of time. For Ruby in particular, we want to call `ddprof_ffi_ProfileExporterV3_send` from a Ruby thread, but there needs to be a way to interrupt a long-running `send` in case the application wants to exit (e.g. the user pressed ctrl+c). Without a way to interrupt a `send`, the Ruby VM will hang until the timeout is hit. To fix this, we make use of tokio's `CancellationToken`: this concurrency utility is built so that it can be used to signal from one thread to another that we want to return early. **NOTE**: I'm marking this PR as WIP since I need some help with the lifetime of the `CancellationToken`: right now it's getting dropped by the functions that use it, whereas it should only be dropped when `ddprof_ffi_CancellationToken_drop` manually does it.

The insight needed to fix my first attempt at implementing cancellation is that the `Box` type automatically drops whatever it was pointing to when the current scope ends. Thus, to leak something out of Rust's control, we need to use `Box::into_raw(Box::new(ThingToBeLeaked()))`, which returns a `*mut ThingToBeLeaked` aka a raw, unsafe pointer. Then, to borrow that something for use, we can use `unsafe { raw_pointer.as_ref() }` to get a `&ThinkToBeLeaked` back. Finally, when we want to free that thing again, we can turn it back into a box; here I'm doing it implicitly, but one other way is to use `Box::from_raw`. Then `drop` can be used again, explicitly or implicitly.

ivoanjo · 2022-04-12T14:47:40Z

@morrisonlevi @paullegranddc can you take a second look?

With a cooler head and a few more learnings from the past days I dug back into the Rust book and figured out how to get the correct lifetime for the CancellationToken instances. I've also updated the C++ example so that anyone can play with it as well.

morrisonlevi · 2022-04-13T18:20:08Z

+    if cancel_reference.is_cancelled() {
+        return false;
+    }
+    cancel_reference.cancel();
+
+    true
+}


This is a race condition, yeah? Some other thread could have cancelled between the check and the cancel. Can we call cancel even if it's been cancelled? If hope so, and if so I'd prefer that and return nothing instead of bool.

Yeah, looking at the code, cancelling twice doesn't do anything. It's just an atomically updated flag.

Yup, this is inherently a race -- the early return is not an attempt at cancelling exactly once (the CancellationToken already does that as Paul pointed out), I'm just trying to add some visibility to the caller.

Happy to change this to not return anything, if we feel like this return is not useful.

I don't think I would use it, but if you think you would in Ruby for some diagnostic or something go ahead.

…use it A cloned CancellationToken is connected to the CancellationToken it was created from. Either the cloned or the original token can be used to cancel or provided as arguments to send. The useful part is that they have independent lifetimes and can be dropped separately. Thus, it's possible to do something like: ```c cancel_t1 = ddprof_ffi_CancellationToken_new(); cancel_t2 = ddprof_ffi_CancellationToken_clone(cancel_t1); // On thread t1: ddprof_ffi_ProfileExporterV3_send(..., cancel_t1); ddprof_ffi_CancellationToken_drop(cancel_t1); // On thread t2: ddprof_ffi_CancellationToken_cancel(cancel_t2); ddprof_ffi_CancellationToken_drop(cancel_t2); ``` Without clone, both t1 and t2 would need to synchronize to make sure neither was using the cancel before it could be dropped. With clone, there is no need for such synchronization, both threads have their own cancel and should drop that cancel after they are done with it.

paullegranddc · 2022-04-14T12:01:41Z

+/// Without clone, both t1 and t2 would need to synchronize to make sure neither was using the cancel
+/// before it could be dropped. With clone, there is no need for such synchronization, both threads
+/// have their own cancel and should drop that cancel after they are done with it.
+pub extern "C" fn ddprof_ffi_CancellationToken_clone(


Nice documentation

Up until now, `ddprof_ffi_ProfileExporterV3_send` was blocking and nothing on the Ruby side could interrupt it. This meant that if reporting a profile was slow/hung, then pressing ctrl+c or calling `Thread#kill` would do nothing -- send would only return after it hit the configured timeout (which by default is 30 seconds). This was very annoying if we're talking about an app that is trying to shut down and is being delayed by a report that is taking longer than expected. To fix this, we make use of the `CancellationToken` added in <DataDog/libddprof#45> to enable the Ruby VM to signal to libddprof that it should cancel an in-progress `send`. Thus, as the added spec shows, we can make `send` return early by interrupting the thread, rather than needing to wait until the timeout was hit.

paullegranddc reviewed Apr 4, 2022

View reviewed changes

Comment thread ddprof-exporter/src/lib.rs Outdated

morrisonlevi reviewed Apr 12, 2022

View reviewed changes

Comment thread ddprof-ffi/src/exporter.rs Outdated

Comment thread ddprof-ffi/src/exporter.rs Outdated

ivoanjo added 3 commits April 12, 2022 13:48

Fix typos in error constant and message

8ecc6ea

Simplify send function

48dbaea

ivoanjo force-pushed the ivoanjo/send-cancellation branch from c67a7a7 to 38646dd Compare April 12, 2022 14:25

ivoanjo changed the title ~~WIP: Asynchronous cancellation for ddprof_ffi_ProfileExporterV3_send~~ Asynchronous cancellation for ddprof_ffi_ProfileExporterV3_send Apr 12, 2022

ivoanjo force-pushed the ivoanjo/send-cancellation branch from 927826a to f5ab2b7 Compare April 12, 2022 14:36

ivoanjo added 3 commits April 12, 2022 15:37

Remove irrelevant comment from ffi header

2c12e69

Run cargo fmt

b59f166

ivoanjo force-pushed the ivoanjo/send-cancellation branch from f5ab2b7 to b59f166 Compare April 12, 2022 14:37

Minor tweak to comment

4981280

ivoanjo force-pushed the ivoanjo/send-cancellation branch from 69e9c99 to 4981280 Compare April 12, 2022 14:39

Make linter happy

a18ffa6

paullegranddc reviewed Apr 13, 2022

View reviewed changes

Comment thread ddprof-exporter/src/lib.rs Outdated

Comment thread ddprof-exporter/src/lib.rs Outdated

Comment thread ddprof-exporter/src/lib.rs Outdated

Comment thread examples/ffi/exporter.cpp

morrisonlevi reviewed Apr 13, 2022

View reviewed changes

Comment thread ddprof-ffi/src/exporter.rs

morrisonlevi reviewed Apr 13, 2022

View reviewed changes

morrisonlevi and others added 6 commits April 13, 2022 19:31

Add CancellationToken for sending HTTP requests

e5e4db3

Merge branch main into ivoanjo/send-cancellation

a6ab7b0

Fix format

3a91cac

Fix name on Windows

ef10657

Make CancellationToken optional when calling Request::send

abbf76c

paullegranddc approved these changes Apr 14, 2022

View reviewed changes

morrisonlevi approved these changes Apr 14, 2022

View reviewed changes

ivoanjo merged commit ac75ab9 into main Apr 14, 2022

ivoanjo deleted the ivoanjo/send-cancellation branch April 14, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Asynchronous cancellation for ddprof_ffi_ProfileExporterV3_send#45

Asynchronous cancellation for ddprof_ffi_ProfileExporterV3_send#45
ivoanjo merged 14 commits into
mainfrom
ivoanjo/send-cancellation

ivoanjo commented Apr 4, 2022 •

edited

Loading

Uh oh!

Uh oh!

morrisonlevi left a comment

Uh oh!

Uh oh!

Uh oh!

ivoanjo commented Apr 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

morrisonlevi Apr 13, 2022

Uh oh!

paullegranddc Apr 13, 2022

Uh oh!

ivoanjo Apr 14, 2022

Uh oh!

morrisonlevi Apr 14, 2022

Uh oh!

paullegranddc Apr 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ivoanjo commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

How to test the change?

Uh oh!

Uh oh!

morrisonlevi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ivoanjo commented Apr 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

morrisonlevi Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

paullegranddc Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

ivoanjo Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

morrisonlevi Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

paullegranddc Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ivoanjo commented Apr 4, 2022 •

edited

Loading