Skip to content

feat: add http client #28

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

Merged
merged 6 commits into from
Apr 14, 2024
Merged

Conversation

ZacHooper
Copy link
Contributor

Noticed that someone else recently pushed a commit with an HTTP client. It looks like I've taken a slightly different approach, so making a PR so it's hopefully not wasted effort 😄. Might be able to merge them a bit or just close mine.

Mojo HTTP Client

  • Added a HTTP client using only Mojo and libc external calls.
  • Adds a fix for the addrinfo struct on Macos. Previously, making the getaddrinfo call would return ip address.
    • Thanks to @thatstoasty for the heads up on the fix for the issue.

Haven't really tested beyond get requests, but is working making requests to httpbin.org and google.com currently.

Comment on lines +269 to +301
fn get_ip_address(self, host: String) raises -> in_addr:
var host_ptr = to_char_ptr(host)
var servinfo = Pointer[Self]().alloc(1)
servinfo.store(Self())

var hints = Self()
hints.ai_family = AF_INET
hints.ai_socktype = SOCK_STREAM
hints.ai_flags = AI_PASSIVE

var error = getaddrinfo[Self](
host_ptr,
Pointer[UInt8](),
Pointer.address_of(hints),
Pointer.address_of(servinfo),
)
if error != 0:
print("getaddrinfo failed")
raise Error("Failed to get IP address. getaddrinfo failed.")

var addrinfo = servinfo.load()

var ai_addr = addrinfo.ai_addr
if not ai_addr:
print("ai_addr is null")
raise Error(
"Failed to get IP address. getaddrinfo was called successfully, but"
" ai_addr is null."
)

var addr_in = ai_addr.bitcast[sockaddr_in]().load()

return addr_in.sin_addr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is the same in both unix & macos structs. Figure when they add the ability to set default function in a trait this can be moved the trait as it's identical for both.

@thatstoasty
Copy link
Collaborator

I'm glad some of my work on mojo-http-client was able to help with this!

@saviorand
Copy link
Collaborator

Woah, great teamwork! Will review today, I think we can combine this with the other code that was already merged and get an even better implementation 💪

@saviorand
Copy link
Collaborator

@ZacHooper made a couple changes to bring this in line with latest state on main and merge with the other client implementation. What do you think? Love the create_connection function.
I didn't compare the performance of two implementations though. But the current simple test is passing on both.

@ZacHooper
Copy link
Contributor Author

Awesome looks good to me! 😄
Ran a couple requests against httpbin and working well.

Did notice one thing though. Not sure if I'm using the HTTPRequest/URI structs wrong but found I needed to add the port to the URL or it caused compiler error.

Good

var req = HTTPRequest(
        URI("http://www.httpbin.org:80/status/300"),
        req_str,
        RequestHeader(req_str),
    )

Bad

var req = HTTPRequest(
        URI("http://www.httpbin.org/status/300"),
        req_str,
        RequestHeader(req_str),
    )

@SchmittB
Copy link
Contributor

The unified code from the two implementation looks good, it is more robust and better looking than my original PR for sure. Congrats !

@saviorand
Copy link
Collaborator

@ZacHooper just pushed a fix to properly handle URLs without a port and encode requests. Also added a simple test for external calls. Can you try again?
The URI struct needs quite some improvement, haha. Also I realized a decode method for incoming responses would be helpful, not in scope of this PR though.
If everything works I'll merge. Thanks a ton!

@ZacHooper
Copy link
Contributor Author

Yep working great. Much easier to write the request now 😄.

@saviorand saviorand merged commit 32810e4 into Lightbug-HQ:main Apr 14, 2024
@saviorand
Copy link
Collaborator

Perfect! Done merged.

@saviorand saviorand mentioned this pull request Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants