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

Platform Handle abstraction + connection negotiation code #45

Merged
merged 1 commit into from
Oct 25, 2022

Conversation

pawelchcki
Copy link
Contributor

@pawelchcki pawelchcki commented Aug 25, 2022

First PR to merge in the IPC work
with basic framework for safe handling of file descriptors both within the process and across processes.

With the recent stabilization of https://doc.rust-lang.org/std/os/unix/io/struct.OwnedFd.html I've removed some no longer necessary abstractions that basically reimplemented OwnedFd. I've used io-lifetimes crate to backport the implementation to older rust versions.

The two biggest parts of this PR that lay groundwork for the IPC mechanism are:

PlatformHandle<T> - which allows safe, ref-counted sharing of a FD both within the process and also facilitates transferring it between processes.

  • it can be used across threads to send data through a shared socket without using locks
  • avoiding mutexes is important as the process could be forked (e.g. in PHP) at anytime

Liaison trait - which allows IPC to be set up between processes

  • it implements mechanism to establish a shared named socket on the system
  • only one process will take ownership of the listener type on the socket
  • the rest will only connect to it when needed

@pawelchcki pawelchcki requested review from a team as code owners August 25, 2022 13:53
@pawelchcki pawelchcki force-pushed the pawel/platform_abstraction branch 2 times, most recently from 57f8c14 to 2fca4a2 Compare August 31, 2022 13:27
@pawelchcki pawelchcki force-pushed the pawel/platform_abstraction branch 4 times, most recently from 58c0e45 to 1e0474a Compare September 6, 2022 09:25
expected_scheduled_after - Duration::from_millis(1) < scheduled_in
&& scheduled_in < expected_scheduled_after
);
assert!(expected_scheduled_after - Duration::from_millis(5) < scheduled_in);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paullegranddc I had to up the Delta, as this test was failing intermitently for me on my laptop

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Timing tests are flaky in essence if the clock is not an injected dependency sadly

@pawelchcki pawelchcki force-pushed the pawel/platform_abstraction branch 2 times, most recently from 0442b10 to c074d87 Compare September 6, 2022 09:50
Copy link
Contributor

@paullegranddc paullegranddc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, since there is use of the actual code, it's kind of hard to juge the API

ddtelemetry/src/ipc/platform/unix/locks.rs Show resolved Hide resolved
ddtelemetry/src/ipc/platform/unix/locks.rs Show resolved Hide resolved
ddtelemetry/src/ipc/platform/unix/locks.rs Outdated Show resolved Hide resolved
ddtelemetry/src/ipc/platform/unix/locks.rs Outdated Show resolved Hide resolved
ddtelemetry/src/ipc/platform/unix/platform_handle.rs Outdated Show resolved Hide resolved
ddtelemetry/src/ipc/platform/unix/sockets.rs Outdated Show resolved Hide resolved
ddtelemetry/src/ipc/platform/unix/sockets.rs Outdated Show resolved Hide resolved
ddtelemetry/Cargo.toml Outdated Show resolved Hide resolved
ddtelemetry-ffi/cbindgen.toml Show resolved Hide resolved
ddtelemetry-ffi/src/unix.rs Show resolved Hide resolved
expected_scheduled_after - Duration::from_millis(1) < scheduled_in
&& scheduled_in < expected_scheduled_after
);
assert!(expected_scheduled_after - Duration::from_millis(5) < scheduled_in);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Timing tests are flaky in essence if the clock is not an injected dependency sadly

ddtelemetry/src/ipc/sidecar/unix.rs Outdated Show resolved Hide resolved
ddtelemetry/src/ipc/sidecar/unix.rs Outdated Show resolved Hide resolved
Includes tools for safe handling of file descriptors, locks and forking
a daemon subprocess / sidecar.

With the recent stabilization of https://doc.rust-lang.org/std/os/unix/io/struct.OwnedFd.html I've removed some no longer necessary abstractions that basically reimplemented OwnedFd. I've used io-lifetimes crate to backport the implementation to older rust versions.

The two biggest parts of this PR that lay groundwork for the IPC mechanism are:

PlatformHandle<T> - which allows safe, ref-counted sharing of a FD both within the process and also facilitates transferring it between processes.

it can be used across threads to send data through a shared socket without using locks
avoiding mutexes is important as the process could be forked (e.g. in PHP) at anytime
Liaison trait - which allows IPC to be set up between processes

it implements mechanism to establish a shared named socket on the system
only one process will take ownership of the listener type on the socket
the rest will only connect to it when needed
@pawelchcki pawelchcki merged commit 58b6edc into main Oct 25, 2022
@pawelchcki pawelchcki deleted the pawel/platform_abstraction branch October 25, 2022 12:30
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

Successfully merging this pull request may close these issues.

2 participants