On 7/10/2024 6:16 AM, Roman Arutyunyan wrote:
Hi,
On Thu, Jun 13, 2024 at 03:29:03PM -0700, Aleksei Bavshin wrote:
# HG changeset patch
# User Aleksei Bavshin <a.bavs...@nginx.com>
# Date 1712181327 25200
# Wed Apr 03 14:55:27 2024 -0700
# Node ID 375fa42f1a6010692a8782c4f03c6ad465d3f7f7
# Parent 8c8d8118c7ac0a0426f48dbfed94e279dddff992
Upstream: disable re-resolve functionality on Windows.
Following features are currently not implemented on Windows, making re-resolve
functionality unsafe to use:
* 'noreuse' shared zones that are re-created on each configuration reload.
The work scheduling logic is not prepared to handle simultaneous access to
the shared zone from multiple generations of the worker processes.
I don't see a problem here. Could you please elaborate.
Now I recall that I could've been running a modified version with
different handling of `noreuse` when I wrote that.
The problem is still valid; assuming that `noreuse` is simply ignored on
Windows, we could've get the following behavior:
A new cycle is created on configuration reload, and an existing shared
zone is referenced by the new cycle. Depending on how we handle that,
`ngx_http_upstream_init_zone` in the new cycle either will find an
existing set of peers (still actively used by a previous cycle) and
overwrite host->event fields with worker-local pointers, or will
allocate a new `ngx_http_upstream_rr_peers_t` and eventually exhausts
the zone memory.
Thankfully, neither option is possible and `noreuse` does something on
Windows platform: an upstream zone in the configuration will completely
break configuration reload with
2024/07/10 13:13:12 [alert] 41332#45116: MapViewOfFileEx(65536,
2EFE0000) of file mapping "upstream_zone" failed (487: Attempt to access
invalid address)
What's happening there is that we see the 'noreuse' flag and attempt to
allocate a new shared zone. Due to a platform-specific implementation
detail, we get `ERROR_ALREADY_EXISTS` and proceed with the existing file
mapping object mapped at a different address.
Next, the code added in af7eba90645d will notice that the newly mapped
address does not match the expected address of an existing zone and will
attempt to remap it at the original address via ngx_shm_remap.
As we're in the master process and the original mapping still exists at
that address, `MapViewOfFileEx` would fail and abort the configuration
reload.
Note that you don't even need the patches in this series to reproduce that.
***
Overall, I'm convinced that the problem is not in the resolver but in
the `noreuse` zone type we use in upstreams. And we already allow these
on Windows, so the ship has sailed.
I saw a previous idea to resolve that by allocating `noreuse` zones with
a name unique to the config generation and tried implementing that, but
it wasn't beautiful.
Thus, I'm retracting this patch. Let's just update the Windows platform
limitations in the documentation instead.
* 'ngx_worker' identification.
It is possible to configure multiple worker processes on Windows, even if
only one would actually handle the traffic. All of the worker processes are
currently identified as process 0, breaking scheduling and locking of the
resolver tasks.
This can be fixed. Patch attached.
Note that the patch is insufficient, as it does not handle respawning.
We should store the ngx_worker value in the ngx_processes and restore
the variable when necessary, or find an alternative way of passing it to
the worker process.
My WIP patch with `cycle->generation` support had an attempt to solve
that with making environment snapshots (GetEnvironmentStrings) and
passing these to the CreateProcess, but that quickly escalated into a
ton of unrelated fixes and I decided to stop.
diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -6327,7 +6327,7 @@ ngx_http_upstream_server(ngx_conf_t *cf,
continue;
}
-#if (NGX_HTTP_UPSTREAM_ZONE)
+#if (NGX_HTTP_UPSTREAM_ZONE && !(NGX_WIN32))
if (ngx_strcmp(value[i].data, "resolve") == 0) {
resolve = 1;
continue;
diff --git a/src/stream/ngx_stream_upstream.c b/src/stream/ngx_stream_upstream.c
--- a/src/stream/ngx_stream_upstream.c
+++ b/src/stream/ngx_stream_upstream.c
@@ -545,7 +545,7 @@ ngx_stream_upstream_server(ngx_conf_t *c
continue;
}
-#if (NGX_STREAM_UPSTREAM_ZONE)
+#if (NGX_STREAM_UPSTREAM_ZONE && !(NGX_WIN32))
if (ngx_strcmp(value[i].data, "resolve") == 0) {
resolve = 1;
continue;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
