Skip to content

multi notifications #18432

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.

Sign up for GitHub

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

Jump to bottom
Closed
icing wants to merge 3 commits into from
Closed

multi notifications #18432

icing wants to merge 3 commits into from

Conversation

@icing
Copy link
Contributor

@icing icing commented Aug 29, 2025 •
edited
Loading

An implementation of the discussion #17817, adding a "notification" feature to the multi handle.

  • Notification types INFO_READ and EASY_DONE implemented
  • Notification types expected to grow. The API requires explicit enabling of a type for it to be collected.
  • Almost zero overhead unless application installs callback and enables some notifications
  • Raising a notification does not fail, avoids adding code paths. A failed add (Out Of Memory as only cause) sets an error state that then later fails the curl_multi_perform() or similar calls.
  • Special multi->in_ntfy_callback flag that allows the applications callback to use most of the libcurl API.

Questions/To be discussed:

  • The age_ms parameter, not sure if it is worth it or if milliseconds is enough.
  • Do we want more notification types?

Update: age_ms has been removed as no use case for this is known.

Some measurements on my macOS box:

> CURL_TEST_EVENT=1 python3 tests/http/scorecard.py  -r --request-count=5000 --request-parallels=6 h2
Requests in parallel to httpd/2.4.65
branch   size  total     6 max [cpu/rss]
master   10KB   5000  5321 r/s [81.4%/16MB]
pr18432  10KB   5000  8186 r/s [97.2%/21MB]

This gain eventually equals out with hight parallelism. Still there with 25, almost no difference with 50 transfers in parallel for HTTP/2. Using HTTP/1.1, the improvement holds:

> CURL_TEST_EVENT=1 python3 tests/http/scorecard.py  -r --request-count=5000 --request-parallels=50 h1
Requests in parallel to httpd/2.4.65
branch   size  total    50 max [cpu/rss]
master   10KB   5000  2925 r/s [93.8%/44MB]
pr18432  10KB   5000  6021 r/s [91.6%/31MB]

Without --test-event, there is no difference really, but performance is average:

> python3 tests/http/scorecard.py  -r --request-count=5000 --request-parallels=50 h1
Requests in parallel to httpd/2.4.65
branch   size  total    50 max [cpu/rss]
master   10KB   5000  4177 r/s [93.8%/44MB]
pr18432  10KB   5000  4210 r/s [91.6%/31MB]

@icing icing added feature-window A merge of this requires an open feature window and removed cmdline tool tests libcurl API CI Continuous Integration labels Aug 29, 2025
@icing
Copy link
Contributor Author

icing commented Aug 29, 2025

CI failure on aws-lc reproduced on master. Trying to address via #18434

@curl curl deleted a comment from testclutch Aug 30, 2025
@icing icing requested a review from bagder August 30, 2025 12:19
@icing icing force-pushed the multi_ntfy branch 2 times, most recently from 462f084 to 3503697 Compare September 1, 2025 09:58
@Ealireza
Copy link

Ealireza commented Sep 22, 2025

Dear @bagder ,
I’ve been checking on this daily for the past three weeks, hoping it would be merged. 🥺
I really need it. Could I kindly ask you to prioritize it?🥹

@icing
Copy link
Contributor Author

icing commented Sep 24, 2025

rebased on current master. /cc @bagder

@icing
Copy link
Contributor Author

icing commented Oct 3, 2025

@Ealireza we had some discussion yesterday on how to merge this feature, should it be experiemental, is the current scope good, etc.

Since you are the first user, it the timing information passed to the callback relevant for you? We are considering removing it in the first versions since we are not sure it is worth it. Feedback appreciated.

@Ealireza
Copy link

Ealireza commented Oct 4, 2025

@Ealireza we had some discussion yesterday on how to merge this feature, should it be experiemental, is the current scope good, etc.

Since you are the first user, it the timing information passed to the callback relevant for you? We are considering removing it in the first versions since we are not sure it is worth it. Feedback appreciated.

Dear @icing
The functionality of this new feature is outstanding.
With the added speed and response time, my app has become exceptional.
But honestly, the timing info didn’t matter to me.

I think removing it could be a good idea since it adds extra processing.
If someone needs timing, they can include a time epoch in the data and derive it themselves.

So, removing it isn’t a bad idea , given that it improves and simplifies processing, I think it’s worth it.

Could you add one more section?

If possible, it would be great to have this feature for WebSockets as well.
At the moment, using WebSockets with curl multi, I haven’t been able to get this system working
Maybe my implementation is wrong.
But if this notify system could also handle WebSockets so it can be used in a multi setup without threads, that would be fantastic.

I’m truly grateful for your effort and appreciate all the hard work.
🙏🏻🌹

icing added 2 commits October 6, 2025 12:17
@icing
multi notifications
6e60e79 
    Add infrastructure to colled and dispatch notifications for transfers
    and the multi handle in general. Applications can register a callback
    and en-/disable notification type the are interested in.

    Without a callback installed, notifications are not collected. Same
    when a notification type has not been enabled.

    Memory allocation failures on adding notifications lead to a general
    multi failure state and result in CURLM_OUT_OF_MEMORY returned from
    curl_multi_perform() and curl_multi_socket*() invocations.
@icing
remove the age_ms parameter from the notify callback
f8e0260
@bagder
Copy link
Member

bagder commented Oct 7, 2025

After discussions we agreed to spell out "notify" instead of "ntfy" in the names, then we merge!

@bagder bagder closed this in 357808f Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bagder bagder bagder approved these changes

Assignees

No one assigned

Labels

CI Continuous Integration cmdline tool feature-window A merge of this requires an open feature window libcurl API tests

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@icing @Ealireza @bagder