Skip to content

Conversation

@karlseguin
Copy link
Collaborator

@karlseguin karlseguin commented Mar 26, 2025

This change was supposed to be pretty minor, but it required larger changes to the rest of the project. I'm not opposed to abandoning the changes here.

The goal was to remove the verbose logging while testing (because the new Http Client does a bit of fuzzing, so this is called a lot more) AND to make the loop self-contained, so that callers didn't have to call the onRecv, onSend and onConnect callbacks (to decrement the events_nb).

-- Unexpected breaking change #1 --
I also decided to add a MemoryPool for the ContextTimeout and ContextCancel. This caused a build failure due to an internal dependency loop. The issue is that the interfaces check requires the Loop, but I made the Loop-type dependent on the JsCallback (because it now has a MemoryPool(ContextTimeout) field which has a JsCallback. But JsCallback isn't available until the interfaces check passes!

-- Unexpected breaking change #2 --
More minor, but I removed the unreachable from the code and made the functions return an error. I think we should avoid having runtime-unreachable as much as possible.

-- Unknown change #1 --
Not clear if the inner IO loop had to be a pointer (io: *IO), but I changed it to io: IO, and everything seems to work.

@krichprollsch
Copy link
Member

I agree with you about the Unexpected breaking change #2 👍

About the deletion of interfaces.zig I agree too, but I'm asking the advice of @francisbouvier.

I think Unknown change #1 is ok 👍

@krichprollsch
Copy link
Member

@karlseguin I'm ok to merge it, but can you prepare the PR to update the browser accordingly?
I would like to merge both at the same time.

@karlseguin
Copy link
Collaborator Author

@karlseguin I'm ok to merge it, but can you prepare the PR to update the browser accordingly? I would like to merge both at the same time.

Actually, can we wait until the http client is merged? I just realized that this commit would requires changes to zig-async-io (the removal of the onRecv, onSend callbacks). I rather wait than make the changes there.

This change was supposed to be pretty minor, but it required larger changes
to the rest of the project.

The goal was to remove the verbose logging while testing (because the new
Http Client does a bit of fuzzing, so this is called _a lot_ more) AND to make
the loop self-contained, so that callers didn't have to call the onRecv, onSend
and onConnect callbacks (to decrement the `events_nb`).

-- Unexpected breaking change lightpanda-io#1 --
I also decided to add a MemoryPool for the IO.Completion used by timeout and
cancel, as well as a MemoryPool for the ContextTimeout and ContextCancel. This
caused a build failure due to an internal dependency loop. The issue is that
the interfaces check requires the Loop, but I made the Loop-type dependent on
the JsCallback (because it now has a MemoryPool(ContextTimeout) field which has
a JsCallback. But JsCallback isn't available until the interfaces check passes!

-- Unexpected breaking change lightpanda-io#2 --
More minor, but I removed the `unrechable` from the code and made the functions
return an error. I think we should avoid having runtime-unreachable as much as
possible.

-- Unknown change lightpanda-io#1 --
Not clear if the inner IO loop had to be a pointer (`io: *IO`), but I changed it
to `io: IO`, and everything seems to work.
@karlseguin karlseguin force-pushed the pool_polish branch 2 times, most recently from 6e89bd8 to 8ef6ca2 Compare March 31, 2025 06:36
@karlseguin
Copy link
Collaborator Author

@karlseguin I'm ok to merge it, but can you prepare the PR to update the browser accordingly? I would like to merge both at the same time.

lightpanda-io/browser#498

@krichprollsch krichprollsch merged commit 64b9b2b into lightpanda-io:main Mar 31, 2025
9 checks passed
@karlseguin karlseguin deleted the pool_polish branch April 1, 2025 09:05
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.

2 participants