[Miniflare 3] Assorted port-related improvements #687
Conversation
|
| // Make sure a new was allocated when `port: 0` was passed to `setOptions()` | ||
| t.not(initialURL.port, "0"); | ||
| t.is(initialURL.port, updatedURL.port); | ||
| t.not(initialURL.port, updatedURL.port); |
There was a problem hiding this comment.
I don't think this is actually what we were shooting for.
I think the previous behaviour of: if option port: 0 was set originally and remained 0 on setOptions, retaining the original port is very helpful and expected.
What we need for wrangler, if option port was set to something other that 0 originally and changed to another number (anything, even 0), the port must change (the bug before was that miniflare retained the previous port, even if both times they were non zero and different)
There was a problem hiding this comment.
So if the previous port was 0, and the new setOptions() port is 0, Miniflare should use the previously allocated port?
There's a small race condition here if the OS allocates that previously allocated port somewhere else, between when the old workerd instance is terminated, and the new workerd instance is started. Admittedly, that's quite unlikely, but it's what I was trying to avoid with this change.
I guess my thinking was if port: 0 is being used (the default for Miniflare's API), I'd rather setOptions()s was guaranteed to be safe. The previous behaviour can be achieved with something like...
const opts: MiniflareOptions = {
port: 0
};
const mf = new Miniflare(opts);
const url = await mf.ready;
// ...
opts.port = parseInt(url.port);
await mf.setOptions(opts);We could maybe have this behaviour if port was undefined, but that might complicate things. 🤔
There was a problem hiding this comment.
I agree that race condition is quite unlikely. More so because the port was recently bound by ourselves so we can assume we can rebind to it
The DX of changing the existing config to reset the config without changes is suboptimal. The user would also have to do the same for the inspectorPort and the unsafeDirectPorts on each subworker which gets even harder.
I made this ticket https://jira.cfdata.org/browse/DEVX-927
There was a problem hiding this comment.
Updated with this behaviour, except for...
unsafeDirectPort: 0: it's not really clear which allocated port we should reuse hereport: undefined/inspectorPort: undefined: in these cases, theundefinedsuggests the user really doesn't care what the port is, so allocating a random one each time should be fine. This also makes the mentioned above race condition impossible in tests where users are unlikely to be specifying ports, but are likely be creating lots ofMiniflareinstances and frequently reloading them.
ec10306
to
9805a71
Compare
We'd like this for Wrangler, so we can use the same Miniflare instance for both the regular proxy worker and the inspector proxy worker. The inspector proxy worker needs to run on its own port as `/json` and `/json/version` are well-known path names used by `chrome:https://inspect`'s service discovery. Otherwise, we'd be able to use `routes`. Note this is an unsafe option as it bypasses Miniflare's entry worker, so won't include any request logging/ pretty errors.
Allows setting `inspectorPort: 0` to get a random inspector port. We'd like this for Wrangler's inspector proxy, so we don't have to use `get-port` and can rely on the OS to give us a random port.
If `port: 0` is passed to `Miniflare#setOptions()`, a new random port will be allocated. Previously, Miniflare would always try to reuse the existing port (even if a non-zero port was passed). That behaviour can be retained by passing `port: (await mf.ready).port` to `setOptions()`. We'd like this for Wrangler, so we can guarantee we always reload on a fresh port.
9805a71
to
8374993
Compare
Hey! 👋 This PR includes 3 changes primarily intended to improve the reliability of
wrangler dev. See the commit descriptions for the rationale behind each change.