Closed TheOtherBrian1 closed 1 year ago
Also, I would like to ask: is there any reason why this function is commented as "Private"? It seems like it shouldn't be.
net._http_collect_response
uses an infinte loop(_await_response
) to wait for a response.
I recall an user was using net._http_collect_response
on triggers, which of course caused timeouts on slow http responses.
So yeah, that function is only meant for development purposes. If we really want to expose sync requests, we need to come up with a C implementation.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Timed out responses are labeled as successful.
Please link any relevant issues here: issue 74
What is the new behavior?
Time out responses are now recognized as "ERRORS"
Additional context
The unexpected test failures in my recent pull requests (1, 2), which only modified documentation, prompted me to investigate the error. The root cause appears to be timeouts during test endpoint calls to https://httpbin.org. If the response exceeds the default 2-second timeout,
net._http_collect_response
returns a misleading success status:When the tests encounter a SUCCESS status, they anticipate the response field to be populated with the tuple
(<status_code>, <headers>, <body>)
. However, because timed-out responses are mislabeled as successful, this field remains unpopulated, leading to discrepancies. One fix would be to adjust the worker.c to yield an 'ERROR' code upon timeouts, but ongoing discussions around a potential breaking update in issue 74 and issue 62 suggest caution.While looking for an alternative solution, I ran two GET requests using the Postman Echo API:
Here are the requests' records in the net._http_response table:
{ "args": { "foo1": "bar1", "foo2": "bar2" }, "headers": { "x-forwarded-proto": "https", "x-forwarded-port": "443", "host": "postman-echo.com", "x-amzn-trace-id": "Root=1-6465a4be-32a2128c278ce40d3879ff2f", "accept": "*/*", "api-key-header": "<API KEY>", "user-agent": "pg_net/0.7.1" }, "url": "https://postman-echo.com/get?foo1=bar1&foo2=bar2" }
I discovered that net._http_collect_response, while determining success a response's status, overlooks error messages and assumes any recorded response denotes a successful execution. My proposed solution inspects the error message field for each request, and if a message is found or the request is absent, the function will return an 'ERROR' status. I modified only two lines within the function (denoted by comments) which should resolve this issue.
Also, I would like to ask: is there any reason why this function is commented as "Private"? It seems like it shouldn't be.