feat: add horse-provider-nghttp2 provider (HTTP/2 + gRPC, Delphi + FPC) - #545
Closed
freitasjca wants to merge 0 commit into
Closed
feat: add horse-provider-nghttp2 provider (HTTP/2 + gRPC, Delphi + FPC)#545freitasjca wants to merge 0 commit into
freitasjca wants to merge 0 commit into
Conversation
Contributor
|
Obrigado pelo trabalho de integração do provider nghttp2. Antes de avaliarmos a decisão arquitetural e aprovarmos este PR, a branch precisa ser atualizada e reduzida ao escopo anunciado. Os PRs #552, #551 e #550 já foram integrados ao
Depois dessa limpeza conseguiremos revisar isoladamente a questão principal: aceitar um provider opt-in com dependência dinâmica de |
freitasjca
added a commit
to freitasjca/horse
that referenced
this pull request
Sep 2, 2026
Adds the opt-in define HORSE_PROVIDER_NGHTTP2 (and strict-mode alias HORSE_PROVIDER_NGHTTP2_REQUIRED) to Horse.pas so the nghttp2 HTTP/2 transport can be selected without touching any existing provider code. horse-provider-nghttp2 supplies TListen, DoStartListenFunc, and DoStopListenFunc; Horse.pas routes to them when the define is set. All other providers (Indy, fphttpserver, CrossSocket, mORMot, ICS, HTTP.sys, epoll, IOCP, host-managed) are completely unaffected. Without the define this file compiles identically to the upstream original on both Delphi and Lazarus/FPC. Supersedes closed PR HashLoad#545.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
HORSE_PROVIDER_NGHTTP2provider-selection support toHorse.pas, following the pattern already used by CrossSocket, mORMot, ICS, epoll and IOCP.3 files, +56 / −7.
Horse.pasis +42 / −0 — nothing is removed or renamed, and every line is inside a conditional. Without the define, compilation is byte-identical to upstream on both compilers.HORSE_NGHTTP2maps to the canonical defineFATALguards for impossible combinations (host-managed + self-hosted, provider mutual exclusivity)THorseProviderresolves toTHorseProviderNghttp2when activeCorrection since this PR was opened
Two things in the original description are now out of date, and both made the ask look larger than it is.
FPC 3.2.2 is supported. The original text said "FPC trunk 3.3.1". That was wrong even then, and it matters here:
.github/workflows/tests.ymlinstalls the compiler withapt-get install -y fpc, which is 3.2.2. The HTTP/2 transport, TLS, the epoll engine, streaming and WebSocket all build and pass on 3.2.2 — verified over 25 stages. Only the gRPC layer needs trunk, because 3.2.2'sRttiunit declares noTCustomAttribute, and it is separable behindHORSE_NGHTTP2_NO_GRPC. Nothing in this PR requires a compiler Horse's CI does not already install.The provider is smaller than it was. The gRPC layer has moved out of
horse-provider-nghttp2intoDelphi-nghttp2(1.5.0–1.7.0). It never referenced a Horse unit — only its unit names did — so the move was a rename. What remains in the provider is the adapter: lifecycle shapes, request/response bridges, context pool, worker pool, WebSocket, stream writer. Structurally the same shape asHorse.Provider.CrossSocket.*.Dependencies
horse-provider-nghttp2≥ 1.7.0 andDelphi-nghttp2≥ 1.7.0, viaboss installlibnghttp2≥ 1.59, dynamically loaded — no link-time dependency, and the framework compiles and links without itValidation
grpcurlagainst the server on both Linux and WindowsFor this PR specifically: compiling without the define produces no change from upstream; with
HORSE_PROVIDER_NGHTTP2it routes toHorse.Provider.Nghttp2; the legacy define behaves identically; and each invalid host/provider combination fails at compile time with the intendedFATAL.Relationship to the other open PRs
These are independent of this one — the hooks compile and route without them — but the provider needs them at runtime, and #552 affects existing users regardless of whether this PR is merged:
Horse.Response.pasomitsHORSE_PROVIDER_NGHTTP2from the guard around the default stream-writer registration.FStreamWriterFactoryis a last-writer-wins class var written from two initialization sections, so the order is decided by the compiler's dependency walk. On FPC trunk the provider's factory happens to win; on 3.2.2 it does not, and every streaming request answers with total silence — no headers, no body, client timeout. One clause. This one is worth taking on its own merits.EAGAINtreated as a disconnect in the WebSocket socket transportFeedBytesthat FPC does not resolveUpgradeheaderIf it helps to take them in an order: #552 first (one clause, no dependency on anything else here), then #549 and #551, then #550, and this one last since it is the one that carries a policy question rather than a defect.
The policy question
This would be Horse's first provider with a third-party runtime dependency.
libnghttp2is dynamically loaded, so the framework still compiles and links without it and a user who never sets the define never encounters it — but the repository's dependency story changes from "none" to "none unless you opt in". That seems worth deciding deliberately rather than absorbing as a side effect of a merge, and I would rather it be discussed here than assumed.Reverting is one commit: the additions are conditional and nothing existing was modified.