Skip to content

feat: Create HTTPTransport#53

Merged
nilslice merged 4 commits intoextism:mainfrom
gjenkins8:create_http_transport
Feb 5, 2025
Merged

feat: Create HTTPTransport#53
nilslice merged 4 commits intoextism:mainfrom
gjenkins8:create_http_transport

Conversation

@gjenkins8
Copy link
Contributor

@gjenkins8 gjenkins8 commented Jan 26, 2025

Create a HTTPTransport that "round trips" go native http request/responses via Extism's internal HTTP request/response mechanism

Signed-off-by: George Jenkins <[email protected]>
Signed-off-by: George Jenkins <[email protected]>
@gjenkins8 gjenkins8 marked this pull request as ready for review January 26, 2025 20:39
@nilslice
Copy link
Member

Thank you @gjenkins8! Letting the CI do its thing - but as for some feedback on the implementation, I think we should move this into something like a http/std.go package outside the pdk package. This way, when no http operations are needed, the net/http stuff should be eliminated by the compiler and can save a lot of bytes from the .wasm output.

Would you be open to that?

@gjenkins8
Copy link
Contributor Author

Thank you @gjenkins8! Letting the CI do its thing - but as for some feedback on the implementation, I think we should move this into something like a http/std.go package outside the pdk package. This way, when no http operations are needed, the net/http stuff should be eliminated by the compiler and can save a lot of bytes from the .wasm output.

Would you be open to that?

Hey, thanks for the feedback. That makes sense to me, I can move. The big potential hiccup, is I am using private methods in the pdk (root) package. Let me see what I can do / will potentially need. ie. I might need to move some of the things in the pdk package into an internal package.

Signed-off-by: George Jenkins <[email protected]>
@gjenkins8
Copy link
Contributor Author

gjenkins8 commented Jan 26, 2025

a743206 is pretty significant. I moved memory related functionality into an internal/memory package, extism host http functionality into an internal/http package, then finally HTTPTransport into an http package.

As you may imagine, a lot of code movement was required for this. Ultimately, I think it might be worth splitting this PR into smaller PRs (refactors vs feature addition), but I've leave this up to reviewer(s) to decide if needed.

edit: I could also imagine there could be different ways to slice and dice these changes e.g. all host / wasmimport functions could be in an "internal/extismhost package. Open to feedback of course

@gjenkins8
Copy link
Contributor Author

gjenkins8 commented Jan 26, 2025

Unscientific datapoints:

this line increases the compiled wasm module size from 2.3Mib to 6.6Mib (big go, GOOS=wasip1):

http.DefaultTransport = &pdkhttp.HTTPTransport{}

and these three lines increase the compiled module size from 2.3Mib to 9.1Mib:

	http.DefaultTransport = &pdkhttp.HTTPTransport{}
	client := http.DefaultClient
	_, err := client.Head("https://jsonplaceholder.typicode.com/todos/1")

So it seems like a worthy goal have to HTTPTransport in its own separate package

Copy link
Contributor

@evacchi evacchi left a comment

Choose a reason for hiding this comment

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

from what I can tell (I am not super-familiar with this codebase) the changes make sense, I'll refer to @mhmd-azeez and/or @nilslice for approval :)

Copy link
Contributor

@mhmd-azeez mhmd-azeez left a comment

Choose a reason for hiding this comment

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

This unlocks a lot of REST client libraries, thank you very much!

@nilslice nilslice merged commit eea8e06 into extism:main Feb 5, 2025
0 of 2 checks passed
@nilslice
Copy link
Member

nilslice commented Feb 5, 2025

will get this out in the next release!

@gjenkins8 gjenkins8 deleted the create_http_transport branch February 7, 2025 00:07
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