During my testing using Alex's playground scripts, I noticed that when 
we restart `linkd` for a seed without changing profile, it fails
due to an error: "Address already in use". This is caused by binding
to UNIX sockets that already exist.

This patch is to unlink the UNIX sockets before binding. If the UNIX
sockets did not exists, it will be no-op.

I tested with the diff and no longer see these errors.

Han Xu (1):
  unlink unix socket before bind

 cli/linkd-lib/src/api/sockets.rs | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.32.0 (Apple Git-132)

---
 cli/linkd-lib/src/api/sockets.rs | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/cli/linkd-lib/src/api/sockets.rs b/cli/linkd-lib/src/api/sockets.rs
index 050c1dd0..6ea13315 100644
--- a/cli/linkd-lib/src/api/sockets.rs
+++ b/cli/linkd-lib/src/api/sockets.rs
@@ -132,8 +132,19 @@ fn env_sockets() -> Result<SyncSockets, Error> {
 fn profile_sockets(profile: &Profile, peer_id: &PeerId) -> Result<SyncSockets, Error> {
     let rpc_socket_path = profile.paths().rpc_socket(peer_id);
     let events_socket_path = profile.paths().events_socket(peer_id);
-    let rpc = StdUnixListener::bind(rpc_socket_path.as_path())?;
-    let events = StdUnixListener::bind(events_socket_path.as_path())?;
+
+    // UNIX socket needs to be unlinked if already exists.
+    nix::unistd::unlink(&rpc_socket_path).ok();
+    let rpc = StdUnixListener::bind(rpc_socket_path.as_path()).map_err(|e| {
+        tracing::error!("bind rpc_socket_path: {:?} error: {}", &rpc_socket_path, &e);
+        e
+    })?;
+    nix::unistd::unlink(&events_socket_path).ok();
+    let events = StdUnixListener::bind(events_socket_path.as_path()).map_err(|e| {
+        tracing::error!("bind events_socket_path: {:?} error: {}", &events_socket_path, &e);
+        e
+    })?;
+
     Ok(SyncSockets {
         rpc,
         events,
-- 
2.32.0 (Apple Git-132)

radicle-link/patches/nixos-latest.yml: FAILED in 10m50s

[unlink UNIX socket before bind][0] from [Han Xu][1]

[0]: https://lists.sr.ht/~radicle-link/dev/patches/34601
[1]: keepsimple@gmail.com

✗ #821420 FAILED radicle-link/patches/nixos-latest.yml https://builds.sr.ht/~radicle-link/job/821420

LGTM

I'll just note that this is being caused by gitoxide under the hood
due to some signal handler shenanigans.

It would be nice to get rid of the signal handles, unix sockets etc.

Doesn't make it much portable I mean... ?

Can't we just link to git2-rs instead of running all these pipes ?

------- Original Message -------
On Friday, August 12th, 2022 at 9:16 PM, Fintan Halpenny <fintan.halpenny@gmail.com> wrote:


> LGTM
> 

> I'll just note that this is being caused by gitoxide under the hood
> due to some signal handler shenanigans.

On 12/08/22 12:16pm, Fintan Halpenny wrote:
> LGTM
>
> I'll just note that this is being caused by gitoxide under the hood
> due to some signal handler shenanigans.

I think we should fix that in this patch. The problem is that gitoxide
intercepts signals. We just need to turn that off. This is just a
question of adding the following line to the start of the `main`
function:

    git_tempfile::force_setup(git_tempfile::SignalHandlerMode::None);

On 12/08/22 11:19am, MissM_signed@protonmail.ch wrote:
> It would be nice to get rid of the signal handles, unix sockets etc.
>
> Doesn't make it much portable I mean... ?

It's not really intended to be portable though. Windows doesn't have
socket activation and so an alternative approach will probably be needed
anyway.

>
> Can't we just link to git2-rs instead of running all these pipes ?

What do you mean by this?

On Fri, Aug 12, 2022 at 5:09 AM Alex Good <alex@memoryandthought.me> wrote:
>
> On 12/08/22 12:16pm, Fintan Halpenny wrote:
> > LGTM
> >
> > I'll just note that this is being caused by gitoxide under the hood
> > due to some signal handler shenanigans.

How does the signal handler impact the UNIX sockets created in
`linkd-lib` node start ?

>
> I think we should fix that in this patch. The problem is that gitoxide
> intercepts signals. We just need to turn that off. This is just a
> question of adding the following line to the start of the `main`
> function:
>
>     git_tempfile::force_setup(git_tempfile::SignalHandlerMode::None);

Does it mean that if we don't have these signal handler, the node will
never use the UNIX sockets?

thanks
Han

On 15/08/22 08:57am, Han wrote:
> On Fri, Aug 12, 2022 at 5:09 AM Alex Good <alex@memoryandthought.me> wrote:
> >
> > On 12/08/22 12:16pm, Fintan Halpenny wrote:
> > > LGTM
> > >
> > > I'll just note that this is being caused by gitoxide under the hood
> > > due to some signal handler shenanigans.
>
> How does the signal handler impact the UNIX sockets created in
> `linkd-lib` node start ?

Gitoxide installs a signal handler which pre-empts the signal handlers
`linkd-lib` installs to unlink the sockets when the application is
killed.

>
> >
> > I think we should fix that in this patch. The problem is that gitoxide
> > intercepts signals. We just need to turn that off. This is just a
> > question of adding the following line to the start of the `main`
> > function:
> >
> >     git_tempfile::force_setup(git_tempfile::SignalHandlerMode::None);
>
> Does it mean that if we don't have these signal handler, the node will
> never use the UNIX sockets?

No not at all. The unix sockets are used to expose an RPC which local
applications can use to interface with `linkd`.

On Mon, Aug 15, 2022 at 9:19 AM Alex Good <alex@memoryandthought.me> wrote:
>
> On 15/08/22 08:57am, Han wrote:
> > On Fri, Aug 12, 2022 at 5:09 AM Alex Good <alex@memoryandthought.me> wrote:
> > >
> > > On 12/08/22 12:16pm, Fintan Halpenny wrote:
> > > > LGTM
> > > >
> > > > I'll just note that this is being caused by gitoxide under the hood
> > > > due to some signal handler shenanigans.
> >
> > How does the signal handler impact the UNIX sockets created in
> > `linkd-lib` node start ?
>
> Gitoxide installs a signal handler which pre-empts the signal handlers
> `linkd-lib` installs to unlink the sockets when the application is
> killed.

I probably missed this. "git grep unlink" didn't show any other unlink
calls except the ones I've added. Probably using a different
syscall/function?

>
> >
> > >
> > > I think we should fix that in this patch. The problem is that gitoxide
> > > intercepts signals. We just need to turn that off. This is just a
> > > question of adding the following line to the start of the `main`
> > > function:
> > >
> > >     git_tempfile::force_setup(git_tempfile::SignalHandlerMode::None);
> >
> > Does it mean that if we don't have these signal handler, the node will
> > never use the UNIX sockets?
>
> No not at all. The unix sockets are used to expose an RPC which local
> applications can use to interface with `linkd`.

Make sense. I will add the line you mentioned. (btw, it seems that
git_tempfile has "setup()", not "force_setup()").

Thanks!
Han

On 15/08/22 10:07am, Han wrote:
> On Mon, Aug 15, 2022 at 9:19 AM Alex Good <alex@memoryandthought.me> wrote:
> >
> > On 15/08/22 08:57am, Han wrote:
> > > On Fri, Aug 12, 2022 at 5:09 AM Alex Good <alex@memoryandthought.me> wrote:
> > > >
> > > > On 12/08/22 12:16pm, Fintan Halpenny wrote:
> > > > > LGTM
> > > > >
> > > > > I'll just note that this is being caused by gitoxide under the hood
> > > > > due to some signal handler shenanigans.
> > >
> > > How does the signal handler impact the UNIX sockets created in
> > > `linkd-lib` node start ?
> >
> > Gitoxide installs a signal handler which pre-empts the signal handlers
> > `linkd-lib` installs to unlink the sockets when the application is
> > killed.
>
> I probably missed this. "git grep unlink" didn't show any other unlink
> calls except the ones I've added. Probably using a different
> syscall/function?

The relevant code is in `linkd-lib/src/api/sockets.rs:53` in the
`cleanup` function.

>
> >
> > >
> > > >
> > > > I think we should fix that in this patch. The problem is that gitoxide
> > > > intercepts signals. We just need to turn that off. This is just a
> > > > question of adding the following line to the start of the `main`
> > > > function:
> > > >
> > > >     git_tempfile::force_setup(git_tempfile::SignalHandlerMode::None);
> > >
> > > Does it mean that if we don't have these signal handler, the node will
> > > never use the UNIX sockets?
> >
> > No not at all. The unix sockets are used to expose an RPC which local
> > applications can use to interface with `linkd`.
>
> Make sense. I will add the line you mentioned. (btw, it seems that
> git_tempfile has "setup()", not "force_setup()").

You might be looking at the wrong version. The version we are depending
on is (I think) 1.0.6.

Thanks for your comments for the v1 patch. I've added a line in `linkd`
main to disable the signal handlers from gitoxide.

I've kept the `unlink` calls before socket bind so that regardless the
signal handling results we can always proceed with the bindings.

Here is a test log of the signal handler part: (press ctrl-c)

2022-08-16T13:33:59.101456Z  INFO linkd_lib::signals: received
termination signal, signal: SignalKind(2)
    at /home/pi/work/radicle-link/cli/linkd-lib/src/signals.rs:24
    in linkd_lib::signals::signals subroutine

2022-08-16T13:33:59.102090Z  INFO linkd_lib::api::sockets: Sockets::cleanup
    at /home/pi/work/radicle-link/cli/linkd-lib/src/api/sockets.rs:54

2022-08-16T13:33:59.102293Z  INFO linkd_lib::protocol: network
endpoint shut down
    at /home/pi/work/radicle-link/cli/linkd-lib/src/protocol.rs:48
    in linkd_lib::protocol::protocol subroutine


Han Xu (2):
  unlink unix socket before bind
  disable gitoxide signle handlers in linkd

 bins/Cargo.lock                  |  1 +
 bins/linkd/Cargo.toml            |  3 +++
 bins/linkd/src/main.rs           |  2 ++
 cli/linkd-lib/src/api/sockets.rs | 16 ++++++++++++++--
 4 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.32.0 (Apple Git-132)

---
 cli/linkd-lib/src/api/sockets.rs | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/cli/linkd-lib/src/api/sockets.rs b/cli/linkd-lib/src/api/sockets.rs
index 050c1dd0..6ea13315 100644
--- a/cli/linkd-lib/src/api/sockets.rs
+++ b/cli/linkd-lib/src/api/sockets.rs
@@ -132,8 +132,19 @@ fn env_sockets() -> Result<SyncSockets, Error> {
 fn profile_sockets(profile: &Profile, peer_id: &PeerId) -> Result<SyncSockets, Error> {
     let rpc_socket_path = profile.paths().rpc_socket(peer_id);
     let events_socket_path = profile.paths().events_socket(peer_id);
-    let rpc = StdUnixListener::bind(rpc_socket_path.as_path())?;
-    let events = StdUnixListener::bind(events_socket_path.as_path())?;
+
+    // UNIX socket needs to be unlinked if already exists.
+    nix::unistd::unlink(&rpc_socket_path).ok();
+    let rpc = StdUnixListener::bind(rpc_socket_path.as_path()).map_err(|e| {
+        tracing::error!("bind rpc_socket_path: {:?} error: {}", &rpc_socket_path, &e);
+        e
+    })?;
+    nix::unistd::unlink(&events_socket_path).ok();
+    let events = StdUnixListener::bind(events_socket_path.as_path()).map_err(|e| {
+        tracing::error!("bind events_socket_path: {:?} error: {}", &events_socket_path, &e);
+        e
+    })?;
+
     Ok(SyncSockets {
         rpc,
         events,
-- 
2.32.0 (Apple Git-132)

---
 bins/Cargo.lock                  | 1 +
 bins/linkd/Cargo.toml            | 3 +++
 bins/linkd/src/main.rs           | 2 ++
 cli/linkd-lib/src/api/sockets.rs | 1 +
 4 files changed, 7 insertions(+)

diff --git a/bins/Cargo.lock b/bins/Cargo.lock
index d5756144..7eac42de 100644
--- a/bins/Cargo.lock
@@ -2024,6 +2024,7 @@ dependencies = [
 name = "linkd"
 version = "0.1.0"
 dependencies = [
+ "git-tempfile",
  "linkd-lib",
  "tokio",
 ]
diff --git a/bins/linkd/Cargo.toml b/bins/linkd/Cargo.toml
index 8789905a..ba980af1 100644
--- a/bins/linkd/Cargo.toml
@@ -22,3 +22,6 @@ features = [ "macros", "process", "rt-multi-thread" ]
 [dependencies.linkd-lib]
 path    = "../../cli/linkd-lib"
 version = "0.1.0"
+
+[dependencies.git-tempfile]
+version = "1.0.6"
diff --git a/bins/linkd/src/main.rs b/bins/linkd/src/main.rs
index e77f6248..48efdf79 100644
--- a/bins/linkd/src/main.rs
@@ -7,6 +7,8 @@ use linkd_lib::node::run;
 
 #[tokio::main]
 async fn main() {
+    // Make sure gitoxide (including git-tempfile) does not override our signal handlers.
+    git_tempfile::force_setup(git_tempfile::SignalHandlerMode::None);
     if let Err(e) = run().await {
         eprintln!("linkd failed: {:?}", e);
     }
diff --git a/cli/linkd-lib/src/api/sockets.rs b/cli/linkd-lib/src/api/sockets.rs
index 6ea13315..1e00984b 100644
--- a/cli/linkd-lib/src/api/sockets.rs
+++ b/cli/linkd-lib/src/api/sockets.rs
@@ -51,6 +51,7 @@ impl Sockets {
     /// this will remove the socket files which were created when the
     /// sockets were loaded.
     pub fn cleanup(&self) -> std::io::Result<()> {
+        tracing::info!("cleanup sockets");
         match &self.open_mode {
             // Do nothing, the file descriptors are cleaned up by the activation framework
             OpenMode::SocketActivated => {},
-- 
2.32.0 (Apple Git-132)

I'm happy with this :)

Thanks Fintan. Let me know if I need to do anything else for this
patch as I'm new to the process.

Han

On Tue, Aug 16, 2022 at 8:29 AM Fintan Halpenny
<fintan.halpenny@gmail.com> wrote:
>
> I'm happy with this :)

Thanks Han!

Master-At: 9147149a3df43c0b765edd78029f5c8255de0e9c