Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent HTTPRequest from polling invalid client #64472

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 15, 2022

Somewhat fixes #60602

The reason why the error mentioned in the issue typically shows up in the first place is because HTTPRequest keeps polling its associated client. However, as a result of exceeding the maximum DNS requests, the client's resolving id is INVALID, but nothing seems to be done on the HTTPRequest's side to handle it, because the HTTPRequest doesn't know this happened.

Now, only this error show up (no edit on my part, this is appropriate):
image

HTTPClientTCP.connect_to_host() now returns ERR_CANT_RESOLVE if the maximum number of DNS queries has been reached, and so does HTTPRequest.request().

Need to update documentation, but I know ERR_BUSY isn't the most ideal error code (it's already used, too). I need someone to suggest an alternative.

@Mickeon Mickeon force-pushed the try-fixing-http-bug branch 3 times, most recently from 3826355 to 63378c2 Compare August 15, 2022 23:52
@Mickeon Mickeon marked this pull request as ready for review August 16, 2022 00:29
@Mickeon Mickeon requested review from a team as code owners August 16, 2022 00:29
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 16, 2022

Reworked this PR, somewhat. Now it returns early, and the HTTPRequest... request isn't even started if the ID is invalid.

@Calinou Calinou added this to the 4.0 milestone Aug 16, 2022
@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Aug 16, 2022
@akien-mga akien-mga requested a review from a team August 30, 2022 10:03
@akien-mga
Copy link
Member

Could you squash the commits? See PR workflow for instructions.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 5, 2022

As I do that, what would the most appropriate error be for this?

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this issue, see my review comments for changes.

scene/main/http_request.cpp Outdated Show resolved Hide resolved
core/io/http_client_tcp.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks! 🏆

@Faless Faless merged commit 7c99911 into godotengine:master Sep 7, 2022
@Mickeon Mickeon deleted the try-fixing-http-bug branch September 7, 2022 07:32
@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 9, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.5.1.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message for reaching maximum number of concurrent DNS queries is not explicit.
4 participants