Hacker News Reader

Hacker News

Comment by lbj | Hacker News Reader

Full thread

lbj

Holy smokes: https://github.com/jkfran/killport/blob/main/src/linux.rs#L8...

View on HN

My other holy smokes moment was the install sh script hosted on bitly

> curl -sL https://bit.ly/killport | sh

A shame it doesn't require "sudo sh" to install.

Hah, I was about to rant about the same. This tool is obviously not battle proved and never run outside the developer's machine.

care to enlighten us mere mortals? why is this a holy smokes?

A loop over all fds across all processes on a box will be very slow on any large machine with, say, a few hundred thousand processes, some of which may have up to hundreds of thousands or millions of FDs open.

Especially since it looks like it's reading the owning process's cmdline per-fd.

AFAIK there's no other ways to do this on Linux: link a given tcp connection to a process. The network uAPIs (netlink or /proc/net/tcp?) will give you an inode, and to link the inode to the PID you need to go through every open fd in /proc/*/fd. ss, fuser or lsof (mentioned in other comments) do this too.

Yup, ss and friends do this, but they don't read "/proc/$pid/cmdline" N times (where N is the number of file descriptors the process has) in the hotloop.

I phrased it poorly. Doing the loop is what it is, but doing the loop and allocating inside (which 'process.cmdline()' does) on every loop is something I'm fairly sure none of the other tools do.

Oh yeah, definitely. In my implementation, even skipping reading comm (once!) if not needed gave me better perf than ss:

https://github.com/anisse/tcpkill/blob/cfd96d5dec438a3722edb...

> Yup, ss and friends do this, but they don't read "/proc/$pid/cmdline" N times (where N is the number of file descriptors the process has) in the hotloop.

This one doesn't either. The code structure could be clearer IMHO, but `kill_processes_by_inode` reads the cmdline within the `if target_inode == inode` block, which breaks out of the `for fd in fds` loop at the end. So it only looks at the cmdline once per process that has the target inode.

That said, if `find_target_inodes` returns n inodes, `kill_processes_by_port` will call `kill_processes_by_inode` n times. It'd be better to find all fds only once and compare each to all the target inodes at once with a hash set (if n might be large) or by bisecting a sorted slice. Multiple inodes per port could happen in a few different ways: different processes listening to the same port on different IPv4/IPv6 addresses, an old-fashioned pre-forked sort of server model, a bunch of individually spawned single-threaded servers listening on the same port via `SO_REUSEPORT`/`SO_REUSEADDR`.

> A loop over all fds across all processes on a box will be very slow on any large machine with, say, a few hundred thousand processes, some of which may have up to hundreds of thousands or millions of FDs open.

Any implementation of this objective has the same limitation, though.

The overly usage of unwrap() is also suspicious. Processes can vanish while you are inspecting them. So there is a good chance that this tool will panic on busy machines.

Reply on news.ycombinator.com