Feature serverselect#110
Conversation
…rgument validation, updates to test.csv and added integeration testing
…tly throughout. Fixed a couple errors
…han pretend, updated testing to cover this and added more robust logging to main script
|
|
||
| launch:{[port] | ||
| / start a bare q listener detached (no script needed - identity is injected over IPC) | ||
| system qbin," -p ",string[port]," -q </dev/null >/dev/null 2>&1 &"; |
There was a problem hiding this comment.
The inuse probe opens a handle with hopen but only closes it on success (the hclose path). When the port is free, hopen throws and 0b is returned without any handle to close — that is fine. However, the success branch returns 1b immediately after hclose, which is correct. The real problem is the timeout: hopen (hp;500) with a 500 ms timeout will block for up to 500 ms per port when scanning for free ports, making the scan of 10*count names = 70 candidates potentially take 35 seconds on a machine where many ports are filtered (rather than refused). A shorter timeout (e.g. 50 ms) would be safer here, or the probe should use a non-blocking connect approach.
| / open a handle, retrying for up to ~10s while the child binds and accepts | ||
| hp:`$":localhost:",string port; | ||
| step:{[hp;h] if[not null h; :h]; system"sleep 0.2"; @[hopen;(hp;500);{0Ni}]}[hp]; | ||
| :50 step/ 0Ni; |
There was a problem hiding this comment.
baseport:$[0<count s:getenvSSPORT; "I"$s; 20000+int$.z.i mod 20000] — when $SSPORT is not set, getenv returns an empty string "", so count s is 0 and the else branch is taken (correct). But when $SSPORT IS set, "I"$s casts a char-vector to an int-vector (type 6h), not a single int (type 6i). findfree then calls start+til 10*n where start is an int-vector, producing a matrix rather than a vector of candidate ports, so inuse each cand will fail at runtime. The fix is "i"$s (lowercase, int atom) or first "I"$s.
There was a problem hiding this comment.
Incorrect, casting using the lowercase "i" will results in ascii for each char instead of int
| pids:{$[null x; 0Ni; @[x;".z.i";{0Ni}]]} each handles; | ||
| / guarantee the fleet is reaped on ANY exit (normal, error-abort, or exit code) | ||
| .z.exit:{teardown[]}; | ||
| if[any null handles; |
There was a problem hiding this comment.
.z.exit is assigned directly (".z.exit:{teardown[]}") rather than via .dotz.set, violating the TorQ convention and potentially silently overwriting an existing handler (e.g. one set by the TorQ framework itself). Use .dotz.set[.z.exit; {teardown[]}]` instead.
There was a problem hiding this comment.
Ran as a standalone test so not necessary, additionally the use of .dotz.set would result in a failure as it is not available here.
| / guarantee the fleet is reaped on ANY exit (normal, error-abort, or exit code) | ||
| .z.exit:{teardown[]}; | ||
| if[any null handles; | ||
| -2 "failed to connect to child processes on ports: ",(" " sv string ports where null handles); |
There was a problem hiding this comment.
If any null handles is true, exit 3 is called. .z.exit fires and teardown is invoked, but handles at that point may contain 0Ni nulls; the line @[hclose;;{}] each handles where not null handles in teardown should be safe. However, the processes that did start but whose handles are null (connect timed out) will only be reaped by the pkill fallback which matches "bin/q [-]p <port>". If qbin is a custom path that does not contain bin/q, the pkill pattern won't match and those children will be orphaned. The pkill pattern should be derived from qbin or use a more robust signal.
There was a problem hiding this comment.
Actual error,
Fixed. processes that started but never connected had no captured pid (came from .z.i), so were only reachable via pkill which a custom $QBIN breaks.
launch now captures the shell pid (& echo $!) -> exact pid for every child, no .z.i reliance.
pkill fallback rekeyed to [-]p -q (unique port + flag), qbin-independent.
| }; | ||
|
|
||
| / launch the fleet, connect, capture each child's own pid (.z.i) for exact-pid teardown | ||
| -1 "selected free ports: ",.Q.s1 ports; |
There was a problem hiding this comment.
connect uses 50 step/ 0Ni (a fixed-point / convergence scan). Because step returns 0Ni (a null int) on failure and an open handle integer on success, convergence will never naturally terminate if the child never comes up — 0Ni ~ 0Ni is 1b so q's fixed-point / would stop after the first failure, returning 0Ni immediately rather than retrying 50 times. The correct idiom for a counted retry is 50 step\ (scan, discard) followed by a check, or a do[50; ...] loop, or ({not null x} step\) with a separate retry limit.
There was a problem hiding this comment.
Incorrect, reviewer has the "/" converge semantics inverted. Correctly calls 50 times
|
|
||
| / ========================================================================= | ||
| hdr"STEP 3 getservers (lookup + attribute match scoring)"; | ||
| chk["lookups=` returns all active";5;count srvsel.getservers[`;`;()!()]]; |
There was a problem hiding this comment.
STEP 9 kills pids 0 (rdb1) and then sleeps 0.3 s before deactivating it. However pids 0 may be 0Ni (if the @[x;".z.i";{0Ni}] capture failed), in which case system"kill 0Ni ..." passes a literal string "0Ni" to the shell — which will fail silently but leave rdb1 alive. The test then deactivates the handle and assumes rdb1 is dead, but subsequent route calls may still reach rdb1 via its open handle. Add a null check before killing, or assert not null pids 0 at the start of STEP 9.
There was a problem hiding this comment.
Added chk["rdb1 pid captured";1b;not null pids 0] at the start of STEP 9. Note the .z.i capture you cite was already replaced with a shell-$! capture in the prior fix from comment on line 98-101, so pids 0 is now essentially always set. Also, routing can't actually reach rdb1 — route selects only active servers and we deactivate it. The real risk the guard closes: a null pid → kill "0Ni" no-ops silently, letting STEP 9 pass while only exercising deactivation rather than a dead backend. Now it fails loudly instead.
| / ========================================================================= | ||
| hdr"STEP 4 selector (strategies on a known table)"; | ||
| seltab:([]serverid:101 102 103i;handle:201 202 203i;lastp:(2024.01.03D0;2024.01.01D0;2024.01.02D0)); | ||
| chk["roundrobin picks oldest lastp";202i;(srvsel.selector[seltab;`roundrobin])`handle]; |
There was a problem hiding this comment.
STEP 9 routes 6 queries after killing rdb1 and storing results as postresp. Then it asserts not rdb1 in postresp. However routereturns ``NONE when `gethandlebytype` returns `()`, and the open handle for rdb1 is still registered (only deactivated). If the dead rdb1 handle is somehow selected before deactivation takes effect (race), or if `hopen` to a dead process returns an error swallowed by the route trap, `route` returns NONE ``. The assertion not NONE in postresp catches that, but the assertion not rdb1 in postrespwould still pass in that case — leaving the route failure undetected in one scenario. This is acceptable defensive test logic, but the ordering ofkill→sleep→setserveractivemeans there is a window where rdb1 is still active and its handle is dead, so the very firstroutecall inpostrespcould hit the dead handle and return ``NONE `` rather than rdb2/`rdb3`, causing `not `NONE in postresp` to fail spuriously.
There was a problem hiding this comment.
Not an error
the reviewer is misreading the ordering. setserveractive[handles 0;0b] runs before the postresp route loop, and as q is single-threaded, there's no race condition and no window where a route call sees rdb1 active-but-dead (that window is entirely between kill and setserveractive, with no routing in it).
| / validate attribute-requirement values are atoms or simple vectors (control keys excepted); | ||
| / a nested/general-list value (type 0h) would otherwise crash the cross matcher with a raw 'type | ||
| reqkeys:key[att] except `servertype`attributetype`besteffort; | ||
| if[count badkeys:reqkeys where 0h=type each att reqkeys; |
There was a problem hiding this comment.
The new validation reqkeys where 0h=type each att reqkeys indexes att by reqkeys (a symbol vector) using indexing-at-depth, which in q returns corresponding values from the dict. type each on the resulting values is correct. However, att reqkeys when reqkeys is a single-element symbol vector returns a list of one value — type each returns a list of one type — so the comparison 0h=type each ... and where work correctly. The edge case to check is when reqkeys is empty (no non-control keys): att reqkeys returns an empty list, type each returns (), 0h=() is (), count badkeys is 0, and the branch is skipped. That is correct. No bug here — this note is for confirmation only (no finding to post).
There was a problem hiding this comment.
Yes this is correct, works just as stated
DIReview Summary1 critical | 7 warning(s) | 0 suggestion(s)
|
| ports:findfree[baseport;count names]; | ||
|
|
||
| launch:{[port] | ||
| / start a bare q listener detached and capture its shell background pid ($!) - so every |
There was a problem hiding this comment.
The return value of launch is now the shell pid string parsed as an int, but if the system call returns an empty list (e.g. on a platform where echo $! is unavailable or the shell strips the output), first of an empty list returns a null, and "I"$ of a null/empty result would silently produce 0Ni — which the rest of the code already guards against, but the failure mode is silent. The real risk is that first system … on some q versions returns a char list (the raw string), not a string atom, so "I"$ parses correctly; however if the command fails entirely, system raises a signal and the @[…;…;{}]-style trap is absent here. Wrap the call in a trap: @[{"I"$first system x}; cmd; {0Ni}] to prevent an uncaught signal from aborting the entire launch phase.
| / fallback is a last resort for the rare case a pid was never captured - keyed on our own | ||
| / unique port and -q flag (NOT the qbin path, which is configurable), so a custom qbin | ||
| / cannot break it. the [-]p bracket keeps the pattern from matching this very kill command. | ||
| live:pids where not null pids; |
There was a problem hiding this comment.
The pkill fallback pattern "[-]p ",string[x]," -q" matches any process with that port number and -q flag, not just processes spawned by this test run. On a shared host another q process started on the same port (possible if findfree races) would be killed. The previous pattern anchored on bin/q to narrow the match; removing that anchor widens it unnecessarily. Reintroduce a narrower anchor (e.g. include the qbin basename or the process group) so the fallback cannot kill unrelated processes.
|
|
||
| / launch the fleet (capturing each child's shell pid for exact-pid teardown), then connect | ||
| -1 "selected free ports: ",.Q.s1 ports; | ||
| pids:launch each ports; |
There was a problem hiding this comment.
pids:launch each ports replaces the previous two-step sequence where pids was populated after connecting and querying .z.i from each child. The new code assigns pids immediately from the shell's $!, which is the pid of the background shell job, not necessarily the pid of the q process itself (on some systems the shell forks an intermediate process). If the shell pid and the q process pid differ, kill <pid> in teardown and in STEP 9 will target the wrong process. Verify that the shell used by system on all target platforms makes $! equal to the q process pid, or revert to querying .z.i over IPC after connecting.
DIReview Summary1 critical | 2 warning(s) | 0 suggestion(s)
|
2 participants
di.serverselect — TorQ Modularisation PR
Summary
Extracts the server-selection logic from TorQ's
.gwnamespace (gatewaylib.qandgateway.q) into a standalone kdb-x module:di.serverselect. The module maintains a registered pool of backend servers and selects from them by servertype or attribute requirements. It satisfies the di.* module contract: dependency injection viainit, a clean exported API, and no hard module dependencies beyond an injected logger.Background
TorQ's gateway uses a
.gwnamespace to track connected backend servers and route queries to them. Server registration, active-flag management, and selection logic were entangled with the gateway's query execution and connection-handling code. This PR extracts the server-selection layer — the pool of registered servers and the logic to pick from it — as an independently loadable and testable unit.This PR is part of the broader TorQ → kdb-x modularisation effort. The extracted module removes the dependency on TorQ's global process framework and can be used in any gateway or proxy that needs to maintain a backend server pool.
Changes
New files
di/serverselect/serverselect.qinit,addserverfull,addserverattr,addserver,setserveractive,getserverstable,addserversfromtable,getservers,selector,getserverbytype,gethandlebytype,gethpbytype,getserveridsand internal helpersdi/serverselect/init.qserverselect.qand declares the export listdi/serverselect/test.csvdi/serverselect/integration.qdi/serverselect/serverselect.mdDifferences from TorQ original
.gwdi.serverselect.lg.o/.lg.ecallslogdependency, required viainit(no fallback).gw.serverstable.z.m.serversmodule-local mutable statedi.serverselect:prefixusesingleton,init[deps]pattern,export:listattributematch(internal); exposed via thegetserversattribmatchcolumn.servers.SERVERShard-coded lookupaddserversfromtable[proctypes;conntable]accepting any connection tableLogging contract (injected dependency)
Logging is an injected dependency, wired via
init— there is no default logger and the module does not loadkx.logitself; initialising the logging framework is the job of the start-up script or the user.initmust be called before any other function.init[deps]takes a single dict carrying the requiredlogdependency (plus any future optional config). It errors immediately (plain signal) ifdepsis not a dict, is missing`log, orlogis not a dict exposing`info`warn`error.normlognormalises the injected logger to a binary`info`warn`error!{[c;m]}dict — each function takes a context symbolcand a message stringm. It accepts either:kx.loginstance ((use\kx.log)[`createLog][]) — detected by itsgetlvl/sinks/fmtskeys; its monadic functions are wrapped to{[c;m]}, folding context in as a"ctx: msg"` prefix; or`info`warn`error!({[c;m]};{[c;m]};{[c;m]})dict — passed through unchanged..z.m.log[\info][`ctx;"msg"](which maps 1:1 with TorQ's.lg.o[`ctx;"msg"]`).raiseerror[ctx;msg]helper that logs via.z.m.log[\error]**then** signals'"di.serverselect: ",ctx,": ",msg`, so every failure is observable in the log as well as thrown.Exported API
Hardening and fixes
updatestatskeyed onserverid, nothandle.handleis not unique (the table is keyed onserverid, and a freed handle can be reused after reconnect). Keying stats updates on the uniqueserveridprevents a single selection from skewinghits/lastpacross every server that happens to share a handle. Covered by a dedicated regression test.raiseerrorso the failure is logged:addserverfull/setserveractivecheck handle (int) and servertype/active-flag types;addserversfromtablechecks the connection table has the requiredw/proctype/attributescolumns.getserversvalidatesnameortype— it must be`servertypeor`procname(whenlookupsis not`); any other value errors rather than silently falling through to aprocnamelookup.selectorempty-table behaviour documented — it returns a null-valued row; the*bytypehelpers guard against this and return()when no active server matches.Test coverage
Two suites, both green.
Unit tests —
test.csv(k4unit), 128 assertionsinit— dependency injectiondeps, missinglog, non-dictlog, log dict missing a key;di.serverselect:error prefixinit— injected logger usedaddserver/addserverattr/addserverfull— row counts, handles, servertypes, procname/hpup population, serverid autoincrement, type-error rejectionsetserveractiveupdatestatsserverid, even when a handle is sharedgetserversattribmatch; servertype/procname filters; null lookups; per-attribute match scoring; invalidnameortyperejectedselectorroundrobin/last/anystrategies; single-row and empty-table behaviour; unknown strategy rejectedgetserverbytype/gethandlebytype/gethpbytype()for unknown type; round-robin rotation;hitsincrementgetserverids— symbol pathgetserverids— attribute pathbesteffortstrict modeaddserversfromtable`ALL; optionalprocname/hpupcolumns; missing-column rejectionIntegration test —
integration.q, 76 assertionsDrives every exported function through a realistic gateway lifecycle (register → activate/deactivate → query → select → bulk-register → error handling →
initre-wiring with a capturing logger), with aPASS/FAILsummary and a non-zero exit code equal to the number of failures.Running the tests
export QPATH=/path/to/kx/mod:/path/to/kdbx-modules/ integration test QPATH=/path/to/kx/mod:/path/to/kdbx-modules q integration.qIntegration notes
di.serverselecthas no hard module dependencies. The injectedlogis required —initthrows if it is not provided, and must be called before any other function.kx.logby passing the wholecreateLog[]instance (srvsel.init[enlist[\log]!enlist (use`kx.log)[`createLog][]]);normlogdetects and adapts it. A stripped 3-key dict of kx.log's monadic functions must **not** be passed (it would bypass detection and fail with'rank`).addserversfromtableaccepts a TorQ.servers.SERVERS-style table directly: columnsw(handle),proctype,attributesare required;procnameandhpupare optional.getserveridssupports the full TorQ gateway attribute-matching contract: cross-product matching (default), independent matching, per-servertype scoping, andbesteffortmode. Its result is consumed by the gateway viainter/:+first each(andrazefor emptiness checks), which is robust to its per-path return shape.di.serverselect:to identify the source in stack traces.Test results screenshot