feat: Pluggable transport interface + NetHTTP default transport. - #357
feat: Pluggable transport interface + NetHTTP default transport.#357sichanyoo wants to merge 13 commits into
Conversation
jterapin
left a comment
There was a problem hiding this comment.
This is a meaningful step toward a cleaner transport boundary, and I appreciate how much groundwork this PR is laying. I still have a couple blocking concerns around abort/session ownership and how explicitly we define the SendHandler contract (we may need to regroup for team discussion), but I think the overall direction is promising.
It would also be helpful to include a PR description for review context. As a note, I’ll be reviewing the specs in a second pass.
alextwoods
left a comment
There was a problem hiding this comment.
Overall I think this generally makes sense, but I have two big areas worth discussing:
- I think we need much more explicit interface/contract definition (see other comments).
- With the new transport model, I think there ends up being a mismatch between the way the SDK's handler stack interacts with responses and the way the transport works. I'm thinking specifically of the response event hooks like
signal_headersandsignal_data. The new transport contract has effectively already inverted these and its weird for Deserialization/unmarshallling/parsers (Whatever we're calling these) to register logic in event handlers rather than implementing the logic in the handler itself.
| resp.signal_headers(status, headers) | ||
| stream.each_chunk { |chunk| resp.signal_data(chunk) } |
There was a problem hiding this comment.
I forget - are we still relying injecting deserialization/unmarshalling logic into these event listeners? Did we ever discuss removing the event based transport interactions and relying on our existing handler based stack for that instead?
There was a problem hiding this comment.
Deserialization logic already relies on handler based stack (Plugins::Protocol installs ParseHandler/ErrorHandler, which
call protocol.parse_data / parse_error and read context.http_response.body
.read synchronously. The protocols (RpcV2Cbor, NoOpProtocol) expose those as
plain methods).
The callback here is only rly used for (unless I missed something) response_target: streaming. Whether we want to keep it for response_target or to fold it in to SendHandler I think can be followup discussion.
There was a problem hiding this comment.
Ah right, wasn't sure how we did that.
The key tension and disconnect in the design still stands though - and I think this goes a bit to what Juli was describing on the SendHandler's contract vs behavior. The each_chunk (pull) vs response.signal_data (push) feels like it creates two different response models.
There was a problem hiding this comment.
Yeah.. that's true. Instead of using the listener API on response, I think we can just fold it into SendHandler by calling status / header / body writes directly from SendHandler. As for specific logic that we DO handle via these handlers, we should be able to relocate them to appropriate places as needed.
Quick approach survey with AI:
Have SendHandler drive the stream directly and expose the lifecycle as plain method calls / return values instead of emitted
events:
def resolve_response(stream, resp)
status, headers = stream.response_headers
resp.status_code = status # plain setters, no signal_headers/emit
resp.headers = headers
stream.each_chunk { |chunk| resp.body.write(chunk) } # write directly, no signal_data
end
Then move the three ResponseTarget concerns to where the information exists:
- Body-IO selection (currently on_headers): SendHandler (or a small collaborator) picks the body IO after reading status/headers
but before draining, consulting context[:response_target]. This is the same decision, just inline instead of via a listener.
- Success close / error cleanup (on_success/on_error): move into the existing ResponseTarget handler's post-@handler.call section —
it already wraps the whole stack, so after the inner call returns it can inspect response.error/status and close or unlink. No
events needed; it's ordinary handler control flow.
- Response#on_error capture: Response can read context.http_response.error directly (the parse/error handlers already set it),
instead of subscribing.
- ParamConverter file close: do it in its own handler's ensure/post-call block instead of on_done.
Tradeoff: SendHandler gains awareness of "there's a body IO to choose," which slightly couples it to the response-target feature.
Cleaner: give SendHandler a tiny seam like resp.body_for(status, headers) that defaults to the existing StringIO and that
ResponseTarget overrides — keeps SendHandler generic while removing the event bus.
|
|
||
| option( | ||
| :transport, | ||
| doc_type: 'Smithy::Client::NetHTTP::Transport', |
There was a problem hiding this comment.
I think this fits in with Juli's comment about the SendHandler contract - I think if we're trying to make this contract understandable we need to be explicit about it - I think for us that means having an abstract base/module that documents the required methods and their contractually required behaviors. Going further, I think we would want a set of shared compliance tests that exercise that interface that any implementions should use to validate their conformance to that interface.
That goes I think both for Transport and Stream (I see right now both have a contract implementation in the net_http namespace), but I think both need tight contractual definition.
There was a problem hiding this comment.
Agreed, and done for this PR:
- lib/smithy-client/transport.rb: abstract Smithy::Client::Transport
documenting the #transmit contract and required behaviors (send before
return, headers available without body consumption, error semantics). - lib/smithy-client/stream.rb: abstract Smithy::Client::Stream documenting
the staged pull model (headers phase, then ordered body, then terminal),
abort/write/close_write, and the concurrency rules. - NetHTTP::Transport and NetHTTP::Stream now subclass these.
- Shared compliance tests: spec/support/transport_contract.rb ("a transport")
and stream_contract.rb ("a stream"), included from the NetHTTP specs via
it_behaves_like. Any future implementation runs the same suite to validate
conformance.
| # headers and after each body chunk so the caller drives reads. | ||
| # @param [Net::HTTPSession] session | ||
| # @param [Net::HTTPRequest] net_request | ||
| def read_response(session, net_request) |
There was a problem hiding this comment.
Was confused reading this for awhile - read_response to me indicates that it reads the response only, but this both initiates the request, sends it and then reads the response.
There was a problem hiding this comment.
Agreed, the name was misleading. Renamed to #perform_exchange (sends the
request and reads the response) with an updated doc.
|
Comment for reference: Inverting response shaping responsibility from SendHandler => Transport will come in follow-up PR to keep this focused on groundwork of interface. That inversion work will allow SDK to be more pluggable, reduce push vs. pull tension in transport interface, and also enable us to seamlessly refactor away the preexisting response listener APIs. |
jterapin
left a comment
There was a problem hiding this comment.
We're almost there. Once you decide whether you'd rather switch to modules or update the before_initialize check to be subclass-friendly - I'll be comfortable approving.
With the condition that we do the follow-up PRs we mentioned in this one.
| # TODO: re-evaluate when we determine min version for smithy-ruby/v4 GA. | ||
| # TODO: remove this patch, the Stream skip-flag that drives it, and its | ||
| # spec once the min supported Ruby ships net-http >= 0.7.0 (i.e. drops | ||
| # Ruby 3.3/3.4). Keyed on the min Ruby bump so it is not forgotten. |
There was a problem hiding this comment.
Thanks! I suspect we might be extracting this out once we get close
Issue #, if available:
5196
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.