Conversation
Erlang's httpc module supports streaming the body of 200 or 206 responses to the calling process or to a file. These changes expose this API.
lpil
left a comment
There was a problem hiding this comment.
Thank you! I've left a bunch of notes inline, let me know if any of it is unclear.
Streaming chunks is the stated use case. By limiting support to this API (i.e, stream:self, once} it removes considerable complexity and enables deterministic API responses that can be correctly typed in gleam
lpil
left a comment
There was a problem hiding this comment.
This is looking a lot nicer, thank you!
It still needs to be changed to use selectors rather than a custom non-composable API for receiving messages. Check out Mug's https://hexdocs.pm/mug/mug.html#receive_next_packet_as_message and https://hexdocs.pm/mug/mug.html#select_tcp_messages for how this can be done.
The new message types are not very clear due to using tuples instead of a descriptive custom type, and the use of charlists makes it difficult for the programmer to use as they have to know how to convert them to strings. Make a custom type for the response messages please, one designed with the Gleam programmer in mind rather than being more faithful to the Erlang APIs are we building on top of 🙏
Thanks again!
Erlang's httpc module supports streaming the body of 200 or 206
responses to the calling process or to a file. These changes expose the
stream:{self, once} API, the stated use case.
Resolves Issue Expose streaming API gleam-lang#31
37a29e3 to
f7b038a
Compare
lpil
left a comment
There was a problem hiding this comment.
Lovely work! This is def getting where we want it to be. I've left more notes inline, thank you
src/gleam/httpc.gleam
Outdated
| /// If you wish to only handle stream messages from one process, then use one | ||
| /// process per asychronous HTTP request. | ||
| /// | ||
| pub fn initialize_stream_selector() -> process.Selector(StreamMessage) { |
There was a problem hiding this comment.
Use the same design as found in the other packages please, so take the selector as an argument, take a mapping function as an argument, and use the naming convention they use. The TCP library is a good reference here.
If you cannot use an existing selector then you would not be able to use it with code that already has a selector (so most actor based code).
if you do not have a mapping function then you can't produce selectors of different types, which means you cannot easily select other types of messages with the selector.
And the naming convention is just to be consistent and to make the API easy to approach for people who have learnt about how to use selectors already.
Erlang's httpc module supports streaming the body of 200 or 206
responses to the calling process or to a file. These changes expose the
stream:{self, once} API, the stated use case.
Resolves Issue Expose streaming API gleam-lang#31
|
Thanks again for your previous comments and suggestions. Is there anything else you'd like to see before accepting this pull request? |
lpil
left a comment
There was a problem hiding this comment.
Thank you! Nearly there, left some API related notes inline
src/gleam/httpc.gleam
Outdated
| VerifyNone | ||
| } | ||
|
|
||
| pub type HttpSocket |
| RawStreamChunk(RequestIdentifier, BitArray) | ||
| RawStreamEnd(RequestIdentifier, List(#(Charlist, Charlist))) | ||
| RawStreamError(RequestIdentifier, HttpError) | ||
| } |
There was a problem hiding this comment.
This looks like it is an implementation detail and should not be in part of the public interface?
There was a problem hiding this comment.
Yes, it's an implementation detail. I've refactored the code to make it private, and, as a consequence, I was able to simplify the configuration of the selector that receives stream messages (See pub fn select_stream_messages() -> process.Selector(StreamMessage))
There was a problem hiding this comment.
How come we construct values of this type and then immediately convert to another type? Why not construct that final type and skip this intermediate one?
| StreamChunk(RequestIdentifier, BitArray) | ||
| StreamEnd(RequestIdentifier, List(#(String, String))) | ||
| StreamError(RequestIdentifier, HttpError) | ||
| } |
There was a problem hiding this comment.
Could you document this type and add labels please 🙏
| } | ||
|
|
||
| /// Send a HTTP stream request of binary data | ||
| /// |
There was a problem hiding this comment.
Could you finish this documentation please 🙏
src/gleam/httpc.gleam
Outdated
| } | ||
|
|
||
| /// Send a HTTP request of unicode data. | ||
| /// Send a synchronus HTTP request of unicode data. |
src/gleam/httpc.gleam
Outdated
| configure() | ||
| |> dispatch_stream_request(req) | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's have just the dispatch function 🙏
There was a problem hiding this comment.
Can you explain more about this? Are you suggesting that I remove pub fn send_stream_request(req: Request(String)) -> Result(RequestIdentifier, HttpError), leaving dispatch_stream_request whenever streaming?
| /// | ||
| /// With the exception of timeout errors, all other errors will be delivered via: | ||
| /// `StreamError(RequestIdentifier, HttpError)`. | ||
| /// |
There was a problem hiding this comment.
This is a complex API, so folks will need examples to understand how to use it. Could you add these please 🙏
There was a problem hiding this comment.
I've added a thorough example to the README
There was a problem hiding this comment.
I also added a simpler example to the documentation above dispatch_stream_request and send_stream_request functions.
test/gleam_httpc_test.gleam
Outdated
| process.selector_receive(selector, 1000) | ||
| assert request_id_ == request_id | ||
|
|
||
| let _nil = httpc.receive_next_stream_message(pid) |
There was a problem hiding this comment.
Remove the let nil_ = prefixes please, including from the documentation
lpil
left a comment
There was a problem hiding this comment.
Oh sorry! I wrote some review comments and then didn't press submit for some reason. Sorry about that
README.md
Outdated
|
|
||
| ## Http streaming requests | ||
|
|
||
| `httpc` supports `stream:{self, once}` mode, which is a **pull-based** approach for |
There was a problem hiding this comment.
What's this stream:{self, once} syntax? I don't recognise it
There was a problem hiding this comment.
Yes, that's Erlang speak :-) Replaced with `Stream(#(Self, Once))
README.md
Outdated
|
|
||
| /// Receive a streamed response from Postman Echo. The number of | ||
| /// stream chunks we receive is 5, as we specfied by the endpoint | ||
| /// |
There was a problem hiding this comment.
Remove the reference to postman please
README.md
Outdated
| // Configure the selector | ||
| let selector = httpc.select_stream_messages() | ||
| let assert Ok(chunks) = | ||
| loop(request_id, selector, process.self(), bit_array.from_string("")) |
There was a problem hiding this comment.
Do not use raw looping please, we don't want people to do that when they should be using an actor
There was a problem hiding this comment.
Revised to use the Actor model. This required a good deal of thought, but it certainly helped me to better understand your approach to OTP.
test/gleam_httpc_test.gleam
Outdated
| let selector = httpc.select_stream_messages() | ||
| let assert Ok(_request_id) = httpc.send_stream_request(req) | ||
| let assert Ok(httpc.StreamError(_request_id, httpc.SocketClosedRemotely)) = | ||
| process.selector_receive(selector, 1000) |
There was a problem hiding this comment.
Use assert process.selector_receive(selector, 1000) == Ok(httpc.StreamError(request_id, httpc.SocketClosedRemotely)) please 🙏
src/gleam_httpc_ffi.erl
Outdated
| coerce_stream_message({http, {ReqId, stream_start, Headers, Pid}}) -> {raw_stream_start, ReqId, Headers, Pid}; | ||
| coerce_stream_message({http, {ReqId, stream, BinBodyPart}}) when is_binary(BinBodyPart) -> {raw_stream_chunk, ReqId, BinBodyPart}; | ||
| coerce_stream_message({http, {ReqId, stream_end, Headers}}) -> {raw_stream_end, ReqId, Headers}; | ||
| coerce_stream_message({http, {ReqId, {error, Reason}}}) -> {raw_stream_error, ReqId, normalise_error(Reason)}. |
There was a problem hiding this comment.
Wrap these long lines before 80 columns please
src/gleam/httpc.gleam
Outdated
| /// | ||
| pub type StreamMessage { | ||
| /// Sent exactly once when the server response begins. The returned `pid` | ||
| /// identifies the process and must be passed to `httpc:stream_next` |
There was a problem hiding this comment.
No Erlang syntax or explaining Erlang APIs in Gleam documentation please.
There was a problem hiding this comment.
Revised to refer to the Gleam function
| let request = request.map(request, bit_array.from_string) | ||
| use request_id <- result.try(async_dispatch_bits(config, request)) | ||
| Ok(request_id) | ||
| } |
There was a problem hiding this comment.
Let's always work with bits and not have a string specialised version, seeing as it is not possible to ensure that the response is a string, as the other string versions do.
src/gleam/httpc.gleam
Outdated
| ) | ||
| /// Sent for every chunk of response data that the worker emits. Each chunk | ||
| /// must be explicitly requested by calling `httpc:stream_next/1` with the pid | ||
| /// supplied in `StreamStart`. |
There was a problem hiding this comment.
Remove the documentation of Erlang internals please. This is documentation for users, not maintainers.
src/gleam/httpc.gleam
Outdated
| pub type StreamMessage { | ||
| /// Sent exactly once when the server response begins. The returned `pid` | ||
| /// identifies the process and must be passed to `httpc:stream_next` | ||
| /// whenever you are ready for the next chunk. |
There was a problem hiding this comment.
What is this process? Is it some internal detail of Erlang's HTTPC? Is it a process the user creates? What happens if we pass a different pid to the stream function?
There was a problem hiding this comment.
Good question! According to the API documentation, you're supposed to use this pid in stream_next_message. However, I tested a different pid (process.self()) in an example, and it still worked correctly. I suggest we stick with the documented behavior (See https://www.erlang.org/doc/apps/inets/httpc.html#request/5)
| RawStreamChunk(RequestIdentifier, BitArray) | ||
| RawStreamEnd(RequestIdentifier, List(#(Charlist, Charlist))) | ||
| RawStreamError(RequestIdentifier, HttpError) | ||
| } |
There was a problem hiding this comment.
How come we construct values of this type and then immediately convert to another type? Why not construct that final type and skip this intermediate one?
|
In the latest version, I leave it to the user to either work with |
* Updated README with a streaming example * Added error case to streaming tests * Removed send_stream from httpc and improved function documentation
|
I think it's possible for a non streaming request to be returned even when streaming. I added this line to the end of coerce_stream_message coerce_stream_message(Any) ->io:format("X is: ~p~n", [Any]). And get this output. |
|
What's the status on this PR? Streaming seems like something that'd be nice to have :-) |
|
@adkelley I noticed this PR is still marked as a draft |
Expose Streaming API
Motivation
Erlang's inets application provides a set of Internet-related services, including the HTTP client
httpc. Thehttpcmodule gives users the option to stream the body of a 200 or 206 response either to the calling process or directly to a file.This pull request exposes that option to
gleam_httpcwhile maintaining full backward compatibility. It supports the stated use casestream:{self, once}, (i.e., streaming chunks) represented by the custom typeStream:None of the other streaming APIs from the Erlang httpc module is supported (e.g., stream:{filename}).
Implementation Details
Several functions that have synchronous equivalents were added to support streaming, including:
dispatch_stream_bits(config: Configuration, req: Request(BitArray))- similar todispatch_bitsfunction, send a HTTP stream request of binary datadispatch_stream_request(config: Configuration, req: Request(String))- similar to thedispatchfunction, send a HTTP stream request of unicode datasend_stream_request(req: Request(String)- similar to thesendfunction, send an HTTP stream request of Unicode data using the default configuration.The function
select_stream_messagesconfigures a selector to receive stream messages. Finally, the functionreceive_next_stream_message(pid: Pid)triggers the next streaming message to be sent to the calling process designated bypid.Testing
Added two new unit tests,
send_stream_request_test, anddispatch_stream_request_testusing the postman streamed response apiDocumentation
stream:{self, once}exampleRelated Issues