[OTP-19158][OTP-19221] whaileee/maint-25/inets/httpc_improvements #8788

Whaileee · 2024-09-06T13:28:52Z

No description provided.

Whaileee · 2024-09-06T13:32:45Z

This pr closes #8538

lib/inets/src/http_client/httpc.erl

lib/inets/src/http_client/httpc_request.erl

lib/inets/src/http_client/httpc_response.erl

lib/inets/src/http_client/httpc.erl

vkatsuba · 2024-09-06T13:50:00Z

lib/inets/src/http_client/httpc.erl

+        unalias(ClientAlias),
+        receive
+            {http, {RequestId, {ok, saved_to_file}}} ->
+                {ok, saved_to_file};
+            {http, {RequestId, {error, Reason}}} ->
+                {error, Reason};
+            {http, {RequestId, {ok, {StatusLine,Headers,BinBody}}}} ->
+                Body = maybe_format_body(BinBody, Options),
+                {ok, {StatusLine, Headers, Body}};
+            {http, {RequestId, {ok, {StatusCode,BinBody}}}} ->


Suggested change

unalias(ClientAlias),

receive

{http, {RequestId, {ok, saved_to_file}}} ->

{ok, saved_to_file};

{http, {RequestId, {error, Reason}}} ->

{error, Reason};

{http, {RequestId, {ok, {StatusLine,Headers,BinBody}}}} ->

Body = maybe_format_body(BinBody, Options),

{ok, {StatusLine, Headers, Body}};

{http, {RequestId, {ok, {StatusCode,BinBody}}}} ->

true = unalias(ClientAlias),

receive

{http, {RequestId, {ok, saved_to_file}}} ->

{ok, saved_to_file};

{http, {RequestId, {error, Reason}}} ->

{error, Reason};

{http, {RequestId, {ok, {StatusLine, Headers, BinBody}}}} ->

Body = maybe_format_body(BinBody, Options),

{ok, {StatusLine, Headers, Body}};

{http, {RequestId, {ok, {StatusCode, BinBody}}}} ->

lib/inets/src/http_client/httpc_handler.erl

github-actions · 2024-09-06T14:21:36Z

CT Test Results

2 files 21 suites 11m 29s ⏱️
334 tests 310 ✅ 23 💤 1 ❌
558 runs 519 ✅ 39 💤 0 ❌

For more details on these failures, see this check.

Results for commit fb19f1c.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

u3s · 2024-09-16T12:43:37Z

lib/inets/doc/src/httpc.xml

@@ -379,6 +379,12 @@
 	              ReplyInfo}</c>.</p>
 	            </item>

+              <tag><c>alias()</c></tag>


is alias() used in parallel to pid() now? or should alias replace pid?
if both are used, maye we should give some hint which one is used when ... or why and when alias is better ?

I don't think alias should replace pid, as they are two separate datatypes, even if they serve a similar purpose. I believe that by looking up, or knowing what exactly alias is, end-user will be able to judge whether to use pid or alias.

u3s · 2024-09-16T12:57:02Z

lib/inets/src/http_client/httpc.erl

@@ -1144,6 +1163,7 @@ request_options([{Key, DefaultVal, Verify} | Defaults], Options, Acc) ->
      BodyFormat  :: string() | binary() | atom(),
      SocketOpt :: term(),
      Receiver :: pid()


Receiver type is repeated several times in this module. Should it always include alias type (i.e. reference())?

This is the new default for sync requests. Users have to specify the receiver when using {sync, false}.

You lost me, the alias should used for the sync request to ensure that you can time it out without the user process receiving a message that it does not expect. The alias should not be something that the user of the inets API is aware of.

u3s · 2024-09-16T13:05:55Z

lib/inets/src/http_client/httpc.erl

+        cancel_request(RequestId),
+        unalias(ClientAlias),
+        receive
+            {http, {RequestId, {ok, saved_to_file}}} ->


write a code comment explaining why 2nd almost the same receive block is repeated in after Timeout?
is it supposed to handle some race condition? did you observe it in test runs?

why not give up and return error instantly?

This is in case the message comes in after Timeout but before unalias, It ensures that the message queue doesn't contain junk that is unreachable.

u3s · 2024-09-17T05:37:10Z

lib/inets/src/http_client/httpc.erl

 	    case httpc_manager:request(Request, profile_name(Profile)) of
 		{ok, RequestId} ->
-		    handle_answer(RequestId, Sync, Options);
+		    handle_answer(RequestId, Receiver, Sync, Options,
+                                 element(#http_options.timeout, HTTPOptions));


indentation

u3s · 2024-09-17T06:26:40Z

lib/inets/src/http_client/httpc.erl

+       (Value) when is_reference(Value) ->
+        ok;


indentation

u3s · 2024-09-17T06:40:27Z

lib/inets/src/http_client/httpc.erl

+        cancel_request(RequestId),
+        unalias(ClientAlias),
+        receive
+            {http, {RequestId, {ok, saved_to_file}}} ->
+                {ok, saved_to_file};
+            {http, {RequestId, {error, Reason}}} ->
+                {error, Reason};
+            {http, {RequestId, {ok, {StatusLine, Headers, BinBody}}}} ->
+                Body = maybe_format_body(BinBody, Options),
+                {ok, {StatusLine, Headers, Body}};
+            {http, {RequestId, {ok, {StatusCode, BinBody}}}} ->
+                Body = maybe_format_body(BinBody, Options),
+                {ok, {StatusCode, Body}}
+        after 0 ->
+            {error, timeout}
+        end


indentation (whole after block)

u3s · 2024-09-17T07:10:44Z

lib/inets/src/http_client/httpc.erl

+        Reference when is_reference(Reference) ->
+            ok;


indentation

u3s · 2024-09-17T07:26:16Z

PR title could be adjusted to be more meaningful

Whaileee added the team:PS Assigned to OTP team PS label Sep 6, 2024

Whaileee requested review from IngelaAndin and u3s September 6, 2024 13:28

Whaileee self-assigned this Sep 6, 2024

Whaileee added the testing currently being tested, tag is used by OTP internal CI label Sep 6, 2024

vkatsuba reviewed Sep 6, 2024

View reviewed changes

lib/inets/src/http_client/httpc.erl Outdated Show resolved Hide resolved

lib/inets/src/http_client/httpc_request.erl Outdated Show resolved Hide resolved

lib/inets/src/http_client/httpc_request.erl Outdated Show resolved Hide resolved

vkatsuba reviewed Sep 6, 2024

View reviewed changes

lib/inets/src/http_client/httpc_response.erl Outdated Show resolved Hide resolved

vkatsuba reviewed Sep 6, 2024

View reviewed changes

lib/inets/src/http_client/httpc.erl Outdated Show resolved Hide resolved

vkatsuba reviewed Sep 6, 2024

View reviewed changes

lib/inets/src/http_client/httpc_handler.erl Outdated Show resolved Hide resolved

lib/inets/src/http_client/httpc_handler.erl Outdated Show resolved Hide resolved

Whaileee force-pushed the whaileee/maint-25/inets/httpc_improvements branch from 0ea1b80 to abf2130 Compare September 9, 2024 09:30

Whaileee changed the base branch from maint-25 to maint September 10, 2024 13:21

Whaileee changed the base branch from maint to maint-25 September 10, 2024 13:22

u3s reviewed Sep 17, 2024

View reviewed changes

Whaileee force-pushed the whaileee/maint-25/inets/httpc_improvements branch 4 times, most recently from fd5fcc0 to 6f4c5e0 Compare September 24, 2024 08:37

Whaileee added 3 commits September 25, 2024 14:03

OTP-19158 httpc enable options for async request

be1bc8e

OTP-19221 httpc timeout on handle_answer

5411867

Inets: formatting fixes

6c07478

Whaileee force-pushed the whaileee/maint-25/inets/httpc_improvements branch from 6f4c5e0 to 6c07478 Compare September 25, 2024 12:05

inets: Fix error in httpc_SUITE

fb19f1c

Whaileee force-pushed the whaileee/maint-25/inets/httpc_improvements branch from 4aaf71d to fb19f1c Compare September 26, 2024 14:12

Whaileee closed this Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OTP-19158][OTP-19221] whaileee/maint-25/inets/httpc_improvements #8788

[OTP-19158][OTP-19221] whaileee/maint-25/inets/httpc_improvements #8788

Whaileee commented Sep 6, 2024

Whaileee commented Sep 6, 2024

vkatsuba Sep 6, 2024

github-actions bot commented Sep 6, 2024 •

edited

Loading

u3s Sep 16, 2024

Whaileee Sep 23, 2024

u3s Sep 16, 2024

Whaileee Sep 23, 2024 •

edited

Loading

IngelaAndin Sep 23, 2024

u3s Sep 16, 2024

Whaileee Sep 23, 2024

u3s Sep 17, 2024

u3s Sep 17, 2024

u3s Sep 17, 2024

u3s Sep 17, 2024

u3s commented Sep 17, 2024

[OTP-19158][OTP-19221] whaileee/maint-25/inets/httpc_improvements #8788

[OTP-19158][OTP-19221] whaileee/maint-25/inets/httpc_improvements #8788

Conversation

Whaileee commented Sep 6, 2024

Whaileee commented Sep 6, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024 • edited Loading

CT Test Results

Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Whaileee Sep 23, 2024 • edited Loading

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

u3s commented Sep 17, 2024

github-actions bot commented Sep 6, 2024 •

edited

Loading

Whaileee Sep 23, 2024 •

edited

Loading