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

Make AbstractCurlBackend async friendly #2012

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

natsukagami
Copy link
Contributor

@natsukagami natsukagami commented Nov 29, 2023

Just some structural changes to make implementing an async Curl backend (e.g. with curl_multi_*) possible as
a subclass of AbstractCurlBackend.
Mainly:

  • Make header lists per-request.
  • Allow a customizable CurlHandle -> F[CurlCode] method, performCurl. This can be done with curl_multi_*
    instead of just curl_easy_perform. Also seems to be the only thing needed to be changed.

We might have to change curl.option calls as well, if we want to properly handle them as monadic calls.

Before submitting pull request:

  • Check if the project compiles by running sbt compile
  • Verify docs compilation by running sbt compileDocs
  • Check if tests pass by running sbt test
  • Format code by running sbt scalafmt

/** A request-specific context, with allocated zones and headers. */
private class Context() {
val zone: Zone = Zone.open()
var headers: CurlList = _
Copy link
Member

Choose a reason for hiding this comment

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

instead of vars, mabye we can prepare the headers/multiPartHeaders parameters in send, and then create an (immutable) instance of Context? E.g. case class Context(zone: Zone, headers: Option[CurlList], multiPartHeaders: Seq[CurlList])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit tricky to do, since setRequestBody is setting up the multi part headers as it goes through the multipart body. I can on the other hand just exposes a addHeader function to create a header that gets cleared when the context is closed.

@adamw
Copy link
Member

adamw commented Nov 29, 2023

Thanks for the PR! :) I left some comments/questions inline

@adamw adamw closed this Nov 29, 2023
@adamw adamw reopened this Nov 29, 2023
@adamw
Copy link
Member

adamw commented Nov 29, 2023

(sorry wrong button ;) )

val curl = CurlApi.init
if (verbose) {
curl.option(Verbose, parameter = true)
}
if (request.tags.nonEmpty) {
monad.error(new UnsupportedOperationException("Tags are not supported"))
return monad.error(new UnsupportedOperationException("Tags are not supported"))
Copy link
Member

Choose a reason for hiding this comment

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

nice catch! :)

@adamw adamw merged commit 7c3b815 into softwaremill:master Nov 30, 2023
12 checks passed
@adamw
Copy link
Member

adamw commented Nov 30, 2023

Thank you! Do you need a release with these changes, or will you continue working on an async curl backend?

@natsukagami
Copy link
Contributor Author

Thanks!

Do you need a release with these changes

Not yet, as I am waiting for scala-native 0.5 and everything else to fall into place ;)

will you continue working on an async curl backend

I have a prototype over at https://github.com/natsukagami/sttp/tree/curl-async-gears , but as the branch name suggests, it currently depends on gears stuff and serves mostly as a demonstration that it's possible. I might come back and write a proper backend later :D

@natsukagami natsukagami deleted the async-curl-backends branch November 30, 2023 11:27
@natsukagami natsukagami restored the async-curl-backends branch November 30, 2023 11:27
@adamw
Copy link
Member

adamw commented Nov 30, 2023

I see, thanks for the info, I'll be on the lookout for more PRs then :)

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