Project

General

Profile

Actions

Feature #2903

closed

gw backend redesign

Added by stbuehler over 5 years ago. Updated over 5 years ago.

Status:
Fixed
Priority:
Low
Category:
mod_proxy
Target version:
ASK QUESTIONS IN Forums:

Description

I'm not happy about the API; mod_proxy casts a pointer to its own plugin_data as gw_plugin_data to gw_check_extension which happens to work because in both structs there is a just one pointer between PLUGIN_DATA and the conf member, and the conf member happens to be or start with gw_plugin_config; this is WAY to complicated and fragile, and I wouldn't be surprised if a compiler handles this mess as undefined behavior (the PLUGIN_DATA magic is bad enough, to reason to make it worse).

mod_proxy (and mod_wstunnel too, and maybe others) have config options that are not part of gw_plugin_config; mod_wstunnel seems to copy them manually (one by one) into the handler context, but mod_proxy didn't (which is why the "Forwarded" stuff isn't working AT ALL, see #2902). Again very fragile.

The traditional way was that the handler_ctx had a pointer to plugin_data; for gw_backend this would need to be a void* obviously, as we don't know the plugin. Options that may be needed later (i.e. after some other connection might have been "patched" into conf) the config need to be copied of course; but it should be copied as a complete struct instead of copying individual members.


Related issues 1 (0 open1 closed)

Related to Bug #2902: mod_proxy's “proxy.forwarded” option seems ignored when used with mod_auth.Fixed2018-08-18Actions
Actions #1

Updated by stbuehler over 5 years ago

  • Related to Bug #2902: mod_proxy's “proxy.forwarded” option seems ignored when used with mod_auth. added
Actions #2

Updated by gstrauss over 5 years ago

The idea is that the gw_plugin_data is like a base class.

As you mentioned, the reason things need to be copied into the handler context is that, for async modules (such as mod_proxy) which do not immediately process and finish handling a request, the p->conf must be copied since p->conf is reused and might change for the next request, which may be running in parallel as lighttpd is communicating with the backend.

In #2902, I overlooked copying a couple members into the handler context when I converted things to the shared backend infrastructure (gw_backend).

Yes, it might be slightly cleaner to copy the whole p->conf into the handler context, but I don't think it that sloppy to simply copy the needed config params as the handler context is set up. Not all of the config processing intermediates are needed in mod_proxy. Since it is not frequent that new members are added, and now that modules have been converted, the new member will not be present in a modules such as mod_proxy until it is copied into the handler context.

Actions #3

Updated by gstrauss over 5 years ago

  • Tracker changed from Bug to Feature
  • Category set to mod_proxy
  • Status changed from New to Patch Pending
  • Priority changed from Normal to Low
  • Target version changed from 1.4.x to 1.4.51

The gw_handler_ctx structure is required to be the first member of a module's handler_ctx if the module has additional config members. Likewise for gw_plugin_config being first member of plugin_config. If the module does not have additional config members, then the module handler_ctx is gw_handler_ctx and the plugin_config is gw_plugin_config.

For persistence during the lifetime of the request to the backend, handler_ctx contains plugin_config (and, again, gw_plugin_config is part of plugin_config). The gw_plugin_config portion is constructed inside gw_backend.c in gw_check_extension(). A full additional copy of plugin_config from p->conf into hctx->conf would result in some unnecessary copies and would overwrite any changes made in gw_check_extension() beyond straight copy. (Currently there are no changes besides copying a portion of the config in gw_check_extension().)

I'll change mod_proxy and mod_wstunnel to copy the entire plugin_config from p->conf to hctx->conf to keep the code simpler, even though it results in some extra copying.

Actions #4

Updated by gstrauss over 5 years ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100
Actions

Also available in: Atom