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

Implement a CurlClient based on FileDescriptorPoller #117

Open
armanbilge opened this issue Jun 13, 2023 · 8 comments · May be fixed by #124
Open

Implement a CurlClient based on FileDescriptorPoller #117

armanbilge opened this issue Jun 13, 2023 · 8 comments · May be fixed by #124
Assignees
Labels
help wanted Extra attention is needed

Comments

@armanbilge
Copy link
Member

Currently to use http4s-curl we use CurlApp which replaces the IORuntime with a CurlRuntime. This means that it is not possible to integrate http4s-curl with FS2-based applications (Ember, Skunk, Lepus) and other native libraries (e.g. SNUnit).

In Cats Effect 3.6 the default runtime offers file descriptor I/O polling that works with all of these libraries. The idea would be to use IOApp as usual and then to construct a Resource[IO, Client[IO]] with a CurlClientBuilder (similar to Ember).

Here's a rough sketch for how to implement a Client[IO] using file descriptor polling:

  1. Create a MapRef[IO, FileDescriptor, (Fiber, Fiber)] to keep track of a dedicated fiber for polling each relevant file descriptor for each type of interest (read interest vs write interest).

  2. Configure the CURLMOPT_SOCKETFUNCTION to get notified about when cURL needs to start/stop monitoring sockets for read/writes. Use these callbacks to start/cancel fibers in the MapRef.
    https://curl.se/libcurl/c/CURLMOPT_SOCKETFUNCTION.html

  3. To add a new socket you should register the file descriptor.

  4. Then, start a fiber looping on read-readiness and another looping on write-readiness. Whenever the socket is ready, the curl_multi_socket_action should be invoked.
    https://curl.se/libcurl/c/curl_multi_socket_action.html

  5. A callback for CURLMOPT_TIMERFUNCTION should also be registered. It should start a fiber that sleeps the requested time and then calls curl_multi_socket_action.
    https://curl.se/libcurl/c/CURLMOPT_TIMERFUNCTION.html

@hnaderi
Copy link
Collaborator

hnaderi commented Jun 13, 2023

@armanbilge Interesting! 😍
Thanks for providing all the details ☺️
I'm currently overwhelmed by the sheer amount of work I have to do in my job, but I'll take care of this one as soon as I can.

@hnaderi hnaderi linked a pull request Jun 29, 2023 that will close this issue
@hnaderi
Copy link
Collaborator

hnaderi commented Jun 29, 2023

I read the documents and implemented some stuff required for supporting curl multi-socket which is the base of this.
I've got a couple of questions:

  1. In this section:

Then, start a fiber looping on read-readiness and another looping on write-readiness. Whenever the socket is ready, the curl_multi_socket_action should be invoked.

I can't understand how this looping needs to be implemented. Libcurl requires to be notified when there are some changes on the FD state, but this API seems to be for detecting the changes themselves by checking the FD? Am I correct or I'm missing something?

Whenever the socket is ready

Or to put it in other words, how to detect if the socket is ready?

  1. How to get a FileDescriptorPoller? Are there constructors somewhere? Currently I just ignored this by getting it in the constructor arguments, but I wanted to know the big picture and how it's going to be in the final builder?

@hnaderi hnaderi self-assigned this Jun 29, 2023
@armanbilge
Copy link
Member Author

I can't understand how this looping needs to be implemented.

pollReadRec and pollWriteRec already implement that loop :) those methods will call the provided function every time the socket becomes ready on reading or writing.

So basically you will want to start a fiber like this:

pollHandle.pollReadRec(()) { _ =>
  IO {
    curl_multi_socket_action(multi_handle, sockfd, CURL_CSELECT_IN, null)
    Left(()) // loop forever
  }
}

and a similar one for the write loop. This is a fiber that will invoke curl_multi_socket_action with the appropriate arguments every time the socket indicates readiness for read/write.


2. How to get a FileDescriptorPoller?

This is the most unstable part of the API, that Daniel and I are still debating over. Here's how to do it currently, but it might change.

https://github.com/typelevel/fs2/blob/40241c8e803a1730fe1390a09dc5ad2504c883fc/io/native/src/main/scala/fs2/io/ioplatform.scala#L45-L52

@hnaderi
Copy link
Collaborator

hnaderi commented Jun 29, 2023

Got it, Thank you!

@armanbilge
Copy link
Member Author

Thanks for your work, don't hesitate if you have other questions!! 🚀

@hnaderi
Copy link
Collaborator

hnaderi commented Jul 4, 2023

The tests are passing for the new runtime, however it needs polishing and some cleaning.
And also one side effect of using this new runtime is that it doesn't support windows.
@armanbilge What do you think about this? I think we can keep both the old runtime and this new implementation using FD poller, in order not to drop the win support.

@armanbilge
Copy link
Member Author

armanbilge commented Jul 4, 2023

The tests are passing for the new runtime

Wow, amazing!! Thanks so much for trying this out.

I think we can keep both the old runtime and this new implementation using FD poller, in order not to drop the win support.

Yes, this is exactly what I'm thinking as well, if it's not too much maintenance burden we should keep both implementations.

Edit:

both the old runtime

Btw, the old runtime can be refactored as a PollingSystem.

@hnaderi
Copy link
Collaborator

hnaderi commented Jul 4, 2023

Btw, the old runtime can be refactored as a PollingSystem.

Yes, I'm gonna do it after cleaning this one. Currently I've just put a nowarn annotation on it so that I can focus on the task at hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants