feat(net): add isPortAvailable - #7204
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7204 +/- ##
========================================
Coverage 94.83% 94.84%
========================================
Files 617 618 +1
Lines 51674 51438 -236
Branches 9350 9305 -45
========================================
- Hits 49007 48785 -222
+ Misses 2121 2112 -9
+ Partials 546 541 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Great work! Looks good to me, Thanks. I'd go for keeping this one small at this time. |
| using _listener = Deno.listen({ | ||
| port, | ||
| hostname: options?.hostname ?? "0.0.0.0", | ||
| }); |
There was a problem hiding this comment.
Seems like "0.0.0.0" is already the default in Deno.ListenOptions.
| using _listener = Deno.listen({ | |
| port, | |
| hostname: options?.hostname ?? "0.0.0.0", | |
| }); | |
| using _listener = Deno.listen({ port, hostname: options?.hostname }); |
There was a problem hiding this comment.
Good catch. The literal hostname: options?.hostname trips exactOptionalPropertyTypes (passing hostname: undefined is not assignable to hostname?: string), so I forward the hostname only when it is set and otherwise omit it, letting Deno.ListenOptions apply its 0.0.0.0 default:
using _listener = Deno.listen(
options?.hostname !== undefined
? { port, hostname: options.hostname }
: { port },
);That drops the redundant literal without widening what gets forwarded to Deno.listen. Updated in the latest push.
1e1db52 to
8b01f2b
Compare
8b01f2b to
c996483
Compare
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks for this, and for working through @timreichen's round of feedback. The implementation is clean: using for the listener, AddrInUse → false with everything else rethrown (which is the right discrimination, and I'm glad PermissionDenied isn't silently folded into "unavailable"), correct unstable_ file naming and @experimental tags for a 1.0.6 package, and no mod.ts re-export. The policy side is all correct.
What I want to settle before this lands is whether we want the API at all. It isn't literal duplication — getAvailablePort() returns a port and can't answer "is this specific port free?" — but it's the same Deno.listen-and-catch primitive, and getAvailablePort's own JSDoc opens with "In most cases, this function is not needed." This function is thinner than that one, and its main use case is the check-then-bind race we already steer people away from. That's an API-surface call rather than a code-review one, so I'd like another maintainer's read.
The inline notes are what I'd want fixed if we do take it.
| * ```ts no-assert ignore | ||
| * import { isPortAvailable } from "@std/net/unstable-is-port-available"; | ||
| * | ||
| * if (isPortAvailable(8080)) { |
There was a problem hiding this comment.
This example demonstrates exactly the pattern the [!IMPORTANT] block thirty lines above tells people not to use — check, then bind separately, with a window in between where another process can take the port.
A warning followed by a worked example of the thing being warned about mostly teaches the example. People copy code blocks; they skim prose.
I'd drop this example entirely, or replace it with the safe shape (Deno.serve({ port: 0 }) and read the assigned port back). If there's a legitimate check-then-bind use case worth showing, it'd need to be one where losing the race is acceptable and the example says so explicitly.
| * > system assign an available port, or use | ||
| * > {@linkcode https://jsr.io/@std/net/doc/get-available-port/~/getAvailablePort | getAvailablePort}. | ||
| * | ||
| * This function requires the `--allow-net` permission. Any error other than |
There was a problem hiding this comment.
Worth documenting here that availability is per-interface, not global — the current wording reads as though a port is simply free or not.
With the default 0.0.0.0, a process already listening on 127.0.0.1:8080 makes this return false on Linux and macOS, but the same situation can return true on Windows, which has different address-conflict rules. So the same code gives different answers per platform, and neither answer is wrong.
Related: localhost resolving to ::1 versus 127.0.0.1 will also change the result, which matters as soon as someone passes hostname: "localhost".
Also worth saying plainly that this is TCP-only. There's no transport option and UDP occupies a separate port namespace, so isPortAvailable(53) says nothing about a DNS server on UDP 53 — the name doesn't hint at that restriction, only the prose does.
| import { assert, assertFalse, assertThrows } from "@std/assert"; | ||
| import { stub } from "@std/testing/mock"; | ||
|
|
||
| Deno.test("isPortAvailable() returns true for an available port", () => { |
There was a problem hiding this comment.
The true case is tautological as written: port: 0 asks the OS for any ephemeral port, which essentially always succeeds, so this passes even if the implementation were wrong.
A test with real signal: listen on port 0, capture the assigned port, close the listener, then assert isPortAvailable(capturedPort) is true. That exercises the actual "this specific port is free" path, and pairs nicely with the existing in-use test which covers the inverse.
| * {@linkcode Deno.errors.PermissionDenied} for privileged ports (below 1024) | ||
| * or when the `net` permission has not been granted. | ||
| * | ||
| * @param port The port to check. Use `0` to check whether the operating system |
There was a problem hiding this comment.
Nit: documenting 0 as a supported input is a bit odd, since "can the OS assign an ephemeral port" is essentially always true and isn't a question anyone needs this function to answer. Might be cleaner to leave it undocumented, or reject it, than to bless an input whose answer is a constant.
Closes #7196.
@std/netonly hadgetAvailablePort, so there was no quick way to ask "is this specific port free?" without writing theDeno.listenplus catch yourself. This adds that:It returns
truewhen the TCP port can be listened on,falseonDeno.errors.AddrInUse, and rethrows any other error (includingPermissionDeniedfor privileged ports or a missingnetpermission).hostnamedefaults to0.0.0.0. Sincenetis stable, it lives inunstable_is_port_available.tsas the@std/net/unstable-is-port-availableexport and isn't re-exported frommod.ts.The doc comment is upfront about the obvious race: the port can be taken between the check and your own
listen, so it points back toport: 0/getAvailablePortfor cases where you own the listener.Tests cover an available port, a port already in use, the
hostnameoption, and the rethrow path for errors other thanAddrInUse.deno task lint,deno fmt --check, anddeno test -A --docpass locally.One open question: happy to also add
getAvailablePorts(count, options?)here if that would be useful, or keep this PR to the single function.