Add simple http client #1123

bettio · 2024-03-31T13:56:08Z

Add simple http client inspired to mint.

This kind of API works well with memory constrained devices, since it allows to handle chunks and connection events one by one.

TODO after this PR:

Rework default headers handling, such as Host and Content-Lenght
Handle chuncked body with delimiters

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

libs/eavmlib/src/ahttp_client.erl

UncleGrumpy

I am not sure about the format of your specs.

UncleGrumpy · 2024-05-08T14:54:29Z

That comment was about the function specs. Although it does apply to types as well. I did not get the first line highlighted and included in my comment so the context was not very clear.

UncleGrumpy

This looks good to me.

arpunk

Thank you for this 🎉

pguyot

We are lacking tests beyond the example. When trying to test this code beyond a simple redirect on atomvm.net (e.g. to get HTTP errors), I ended up realizing our ssl implementation is not compatible with TLS 1.3 by lack of proper verification options.

pguyot · 2024-05-14T03:59:26Z

libs/eavmlib/src/ahttp_client.erl

+    [{binary, true} | WithActive].
+
+take_parse_headers(Options) ->
+    case proplists:get_value(parse_headers, Options) of


Maybe we should implement lists:keytake/3 to avoid parsing the list twice here.

Indeed, opened #1159.

pguyot · 2024-05-14T04:00:05Z

libs/eavmlib/src/ahttp_client.erl

+
+-define(DEFAULT_WANTED_HEADERS, [<<"Content-Length">>]).
+
+-record(http_client, {host, socket, parser, ref, parse_headers}).


Could we please have types for record fields?

pguyot · 2024-05-14T04:01:58Z

libs/eavmlib/src/ahttp_client.erl

+%% @doc     Makes a http request using given method on provided path.
+%%
+%%          When using methods such as `GET' the body should be omited using either `undefined' or
+%%          `nil' (they are both equivalent).


Why do we have nil? If
undefined is not the only possible value, I would have expected [] or maybe none.

To make it Elixir friendly. nil is like undefined but it has a special meaning in Elixir.

pguyot · 2024-05-14T04:05:56Z

libs/eavmlib/src/ahttp_client.erl

+%%          be provided.
+%%
+%%          `stream' option can be used with `stream_request_body/3' in order to upload a bigger
+%%          binary in streaming mode. This option should be combined with `content-length' header.


Couldn't we stream without knowing the length? If it works we could just change the documentation with s/should be combined/may be combined/

Yes, but it requires chunked uploads with delimiters, that is beyond the scope of this PR. I would work on this with a further set of improvements.

pguyot · 2024-05-14T04:09:14Z

libs/eavmlib/src/ahttp_client.erl

+    case send(Socket, Data) of
+        ok ->
+            Ref = make_ref(),
+            WantedHeaders = ?DEFAULT_WANTED_HEADERS ++ Conn#http_client.parse_headers,


We may end up having content-length twice here if user wanted it. Also it's a little bit confusing, if I ask for a single header (e.g. Location), I end up having two. I don't need Content-Length if I am doing a HEAD request or I expect a Location.

good point, I think I need to improve this API on a further iteration.

pguyot · 2024-05-14T05:30:17Z

libs/eavmlib/src/ahttp_client.erl

+split_header([Name | Tail], HeaderLine) ->
+    NameLen = byte_size(Name),
+    case HeaderLine of
+        <<Name:NameLen/binary, $:, Value/binary>> -> {ok, Name, Value};


We have a case issue here.

pguyot · 2024-05-14T05:35:54Z

libs/eavmlib/src/ahttp_client.erl

+%%
+%%          The updated returned connection should be used in place of the previous one as a
+%%          parameter for the next call.
+%%          The returned reference can be used later for matching responses when using `stream/2'.


I do not understand this usage, and it's not apparent in example.

This is the only place that I find the documentation a bit unclear too. Perhaps inserting a short example here would help clear up any confusion.

it can be used when handling multiple http requests from the same process, each request will have it's own reference.

I tried to clarify it

pguyot · 2024-05-14T06:01:12Z

libs/eavmlib/src/ahttp_client.erl

+    Parser#parser_state{acc = Chunk}.
+
+parse_line(
+    #parser_state{state = undefined} = Parser, <<"HTTP/1.0 ", C:3/binary, " ", _Txt/binary>>


We don't support HTTP 0.9 ? 😁

I was going to go into the investigating http 0.9 rabbit hole, but then I just realized that is past midnight... ☠️

libs/eavmlib/src/ahttp_client.erl

pguyot · 2024-05-14T06:02:46Z

libs/eavmlib/src/ahttp_client.erl

+transform_options(Options) ->
+    WithActive =
+        case proplists:get_value(active, Options) of
+            undefined -> [{active, true} | Options];


Looks like a workaround for ssl default, doesn't it?

Yes and no: we are not implementing the workaround here, but we are allowing it.
Applications using this API must implement the workaround on their side, this cannot be done in other ways.
Anyway supporting both active and passive mode is a good thing, and it should be kept even after the workaround.

UncleGrumpy · 2024-05-14T19:45:03Z

libs/eavmlib/src/ahttp_client.erl

+%% @param   Method a http method such as "GET", "POST", "PUT", etc...
+%% @param   Path the path to the http resource, such as "/"
+%% @param   Headers a list of headers
+%% @param   Body the body that is sent to the server, may be undefined or nil when there is no body


Also whether it is nil or none the exact term should probably be code syntax highlighted with an Erlang (`') code block in the @param definition, so that it is understood to be a atom, and not "empty" or zero... Unless you go with the [] option.

UncleGrumpy · 2024-05-16T17:19:57Z

libs/eavmlib/src/ahttp_client.erl

+%%          The first returned response to a new request is a status `{status, Ref, 200}' for
+%%          a successful response. After that headers and data may follow.
+%%
+%%          Since the response might span multiple socket messages, `stream/3' may be called


Should this be stream/2, or stream_request_body/3? I don't see a stream/3.

petermm · 2024-05-18T08:50:43Z

libs/eavmlib/src/ahttp_client.erl

+            TrimmedValue = trim_right_spaces(LTrimmedValue, byte_size(LTrimmedValue)),
+            UpdatedParser =
+                case Name of
+                    <<"Content-Length">> ->


"Content-Length" is from the server right?

and thus parsing should be case insensitive?

split_header was taking it from a list, so it was guaranteed to have some casing. I clarified it.

Add simple http client that can be used in different situations, such as when performing updates, inspired to Mint Elixir http client. Signed-off-by: Davide Bettio <davide@uninstall.it>

Signed-off-by: Davide Bettio <davide@uninstall.it>

Disable it since `ahttp_client` cannot be run right now. Signed-off-by: Davide Bettio <davide@uninstall.it>

Signed-off-by: Davide Bettio <davide@uninstall.it>

bettio changed the base branch from main to release-0.6 March 31, 2024 13:56

bettio force-pushed the http-client branch from 2bf9d88 to 8b53fdb Compare April 2, 2024 21:44

bettio force-pushed the http-client branch 3 times, most recently from 0bb3026 to 6f6bdf3 Compare May 7, 2024 22:24

UncleGrumpy reviewed May 8, 2024

View reviewed changes

libs/eavmlib/src/ahttp_client.erl Outdated Show resolved Hide resolved

UncleGrumpy reviewed May 8, 2024

View reviewed changes

bettio force-pushed the http-client branch 2 times, most recently from 1e33d44 to 4470de7 Compare May 8, 2024 13:05

bettio force-pushed the http-client branch 6 times, most recently from cb06fc9 to 1f23730 Compare May 13, 2024 21:15

bettio marked this pull request as ready for review May 13, 2024 21:16

bettio force-pushed the http-client branch from 1f23730 to c171f67 Compare May 13, 2024 21:18

UncleGrumpy approved these changes May 13, 2024

View reviewed changes

arpunk approved these changes May 13, 2024

View reviewed changes

pguyot reviewed May 14, 2024

View reviewed changes

UncleGrumpy reviewed May 14, 2024

View reviewed changes

bettio force-pushed the http-client branch from c171f67 to c740f10 Compare May 16, 2024 12:45

UncleGrumpy reviewed May 16, 2024

View reviewed changes

bettio force-pushed the http-client branch from c740f10 to 1c8feaf Compare May 16, 2024 22:33

petermm reviewed May 18, 2024

View reviewed changes

bettio force-pushed the http-client branch from 1c8feaf to f3bbe27 Compare May 19, 2024 17:46

bettio added 3 commits May 22, 2024 00:52

Add simple http client

65aa1b5

Add simple http client that can be used in different situations, such as when performing updates, inspired to Mint Elixir http client. Signed-off-by: Davide Bettio <davide@uninstall.it>

print debug consume_lines

d944557

Signed-off-by: Davide Bettio <davide@uninstall.it>

fix on macOS

1f56113

bettio added 3 commits May 22, 2024 00:52

print debug

f72c349

WASM: temporary disable test_eavmlib

5adc145

Disable it since `ahttp_client` cannot be run right now. Signed-off-by: Davide Bettio <davide@uninstall.it>

More debug

4b7de83

Signed-off-by: Davide Bettio <davide@uninstall.it>

bettio force-pushed the http-client branch 2 times, most recently from c51e040 to af0e539 Compare May 22, 2024 22:41

Debug even more

22ec303

Signed-off-by: Davide Bettio <davide@uninstall.it>

bettio force-pushed the http-client branch from af0e539 to 22ec303 Compare May 22, 2024 22:57

bettio added 2 commits May 23, 2024 23:38

Debug more and more

89062d7

Signed-off-by: Davide Bettio <davide@uninstall.it>

Add erlang:display

9959815

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple http client #1123

Add simple http client #1123

bettio commented Mar 31, 2024 •

edited

UncleGrumpy left a comment

UncleGrumpy commented May 8, 2024 •

edited

UncleGrumpy left a comment

arpunk left a comment

pguyot left a comment

pguyot May 14, 2024

bettio May 16, 2024

pguyot May 14, 2024

bettio May 14, 2024

pguyot May 14, 2024

bettio May 14, 2024

pguyot May 14, 2024

bettio May 16, 2024

pguyot May 14, 2024

bettio May 16, 2024

pguyot May 14, 2024

bettio May 16, 2024

pguyot May 14, 2024

UncleGrumpy May 14, 2024

bettio May 16, 2024

bettio May 16, 2024

pguyot May 14, 2024

bettio May 16, 2024

pguyot May 14, 2024

bettio May 16, 2024

UncleGrumpy May 14, 2024 •

edited

bettio May 16, 2024

UncleGrumpy May 16, 2024 •

edited

bettio May 16, 2024

petermm May 18, 2024

bettio May 19, 2024


		-define(DEFAULT_WANTED_HEADERS, [<<"Content-Length">>]).

		-record(http_client, {host, socket, parser, ref, parse_headers}).

Add simple http client #1123

Are you sure you want to change the base?

Add simple http client #1123

Conversation

bettio commented Mar 31, 2024 • edited

UncleGrumpy left a comment

Choose a reason for hiding this comment

UncleGrumpy commented May 8, 2024 • edited

UncleGrumpy left a comment

Choose a reason for hiding this comment

arpunk left a comment

Choose a reason for hiding this comment

pguyot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UncleGrumpy May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UncleGrumpy May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio commented Mar 31, 2024 •

edited

UncleGrumpy commented May 8, 2024 •

edited

UncleGrumpy May 14, 2024 •

edited

UncleGrumpy May 16, 2024 •

edited