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 poll() the default #2065

Merged
merged 2 commits into from
Feb 18, 2025
Merged

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Feb 17, 2025

poll() is available on:

  • AIX
  • FreeBSD
  • Genode
  • Haiku
  • HP-UX
  • Illumos/Solaris
  • Linux
  • MINIX 3
  • MacOSX
  • OpenBSD
  • Plan 9
  • QNX
  • RTEMS
  • VxWorks
  • Windows
  • Zephyr
  • z/OS USS

poll() is not available on:

  • NonStop OS
  • OpenVMS

select() can still be enabled by defining CPPHTTPLIB_USE_SELECT.

Most users likely won't see any performance improvements (or the contrary), but the WebSockets code may see a minuscule improvement.

select() can still be enabled by defining CPPHTTPLIB_USE_SELECT.
@yhirose
Copy link
Owner

yhirose commented Feb 17, 2025

@falbrechtskirchinger this suggestion sounds good to me.

@alex-cornford, as far as I know, you are the only user that uses OpenVMS. So once I accept this pull request, you will have to define CPPHTTPLIB_USE_SELECT from the next version. :)

@yhirose
Copy link
Owner

yhirose commented Feb 18, 2025

@falbrechtskirchinger I am fine to change the default behavior from select to poll, but select support is still very important to me.

My biggest concern with this pull request is that select is no longer tested at all on GitHub Actions CI. Could you make necessary changes to the GitHub Actions Workflow files to run the unit test suite with select on all the platform? I know it will cause the testing time double, but it's absolutely necessary cost. Thanks!

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Feb 18, 2025

My biggest concern with this pull request is that select is no longer tested at all on GitHub Actions CI. Could you make necessary changes to the GitHub Actions Workflow files to run the unit test suite with select on all the platform? I know it will cause the testing time double, but it's absolutely necessary cost. Thanks!

Agreed. I was worried that poll() hadn't been tested before, so testing both is a good idea. 20 jobs can run in parallel, so test time shouldn't be longer.

@falbrechtskirchinger
Copy link
Contributor Author

@sum01 @abouvier @jimmy-park @Tachi107 I'm sure @yhirose would appreciate you looking over the CMake changes in this PR. I've added an option HTTPLIB_USE_SELECT which defines CPPHTTPLIB_USE_SELECT. Thanks!

@falbrechtskirchinger
Copy link
Contributor Author

@yhirose All builds output the command line now (added /v:m /clp:ShowCommandLine /nologo to Windows builds). You can verify that CPPHTTPLIB_USE_SELECT is defined where needed.

@yhirose
Copy link
Owner

yhirose commented Feb 18, 2025

Looks great!

@yhirose yhirose merged commit 6e73a63 into yhirose:master Feb 18, 2025
8 of 9 checks passed
@Tachi107
Copy link
Contributor

Poll is already enabled in the Debian cpp-httplib package, so it is already widely deployed :)

Note to self (or to whoever wants to do so): the option should also be added to the meson build scripts

@falbrechtskirchinger
Copy link
Contributor Author

Poll is already enabled in the Debian cpp-httplib package, so it is already widely deployed :)

Good to hear. Shockingly the Gentoo ebuild doesn't enable it.

Note to self (or to whoever wants to do so): the option should also be added to the meson build scripts

I didn't care enough to add an option for the archaic select(). Is meson even available on any OS that doesn't have poll()? Does meson run without poll()? 😆

@falbrechtskirchinger falbrechtskirchinger deleted the default-poll branch February 18, 2025 12:07
@Tachi107
Copy link
Contributor

Tachi107 commented Feb 18, 2025 via email

@alex-cornford
Copy link
Contributor

@falbrechtskirchinger this suggestion sounds good to me.

@alex-cornford, as far as I know, you are the only user that uses OpenVMS. So once I accept this pull request, you will have to define CPPHTTPLIB_USE_SELECT from the next version. :)

@yhirose OpenVMS builds and runs with and without CPPHTTPLIB_USE_SELECT defined.

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Feb 19, 2025

@falbrechtskirchinger this suggestion sounds good to me.
@alex-cornford, as far as I know, you are the only user that uses OpenVMS. So once I accept this pull request, you will have to define CPPHTTPLIB_USE_SELECT from the next version. :)

@yhirose OpenVMS builds and runs with and without CPPHTTPLIB_USE_SELECT defined.

@alex-cornford Great! So, am I just misinformed then and OpenVMS does have poll() or did I make a mistake? Adding some print statements to check...

Edit: No, it's definitely using poll() by default and select() with CPPHTTPLIB_USE_SELECT defined. 👍

@alex-cornford
Copy link
Contributor

@falbrechtskirchinger this suggestion sounds good to me.
@alex-cornford, as far as I know, you are the only user that uses OpenVMS. So once I accept this pull request, you will have to define CPPHTTPLIB_USE_SELECT from the next version. :)

@yhirose OpenVMS builds and runs with and without CPPHTTPLIB_USE_SELECT defined.

@alex-cornford Great! So, am I just misinformed then and OpenVMS does have poll() or did I make a mistake? Adding some print statements to check...

Edit: No, it's definitely using poll() by default and select() with CPPHTTPLIB_USE_SELECT defined. 👍

The OpenVMS C++ compiler supports poll() and select(). I don't know how it works internally.

@yhirose
Copy link
Owner

yhirose commented Feb 19, 2025

@falbrechtskirchinger according to your list, 'NonStop OS' is the only environment and I don't think there is any user using cpp-httplib on the OS. So I am now thinking of going further, which means removing select() completely and using poll() only. It can simplify the code and reduce my maintenance cost. What do you think?

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Feb 20, 2025

@falbrechtskirchinger according to your list, 'NonStop OS' is the only environment and I don't think there is any user using cpp-httplib on the OS. So I am now thinking of going further, which means removing select() completely and using poll() only. It can simplify the code and reduce my maintenance cost. What do you think?

@yhirose I have double-checked my list since I was wrong about OpenVMS.

  • VxWorks only has select() (it has several functions including the word poll, which threw off my search).
  • I've re-confirmed some of the other ones, I was less sure about: RTEMS (only since 2021), QNX, Zephyr
  • Genode and NonStop OS have neither select() nor poll().

So, it's only VxWorks, AFAICT. But, cpp-httplib doesn't build on VxWorks right now anyway:

#  ifdef VXWORKS
#   include <sockLib.h>
#   include <selectLib.h>

https://www.ee.torontomu.ca/~courses/ee8205/Data-Sheets/Tornado-VxWorks/vxworks/ref/selectLib.html#select

Removing select() doesn't seem unreasonable. Just give me the go-ahead, and I'll submit a PR.

@yhirose
Copy link
Owner

yhirose commented Feb 20, 2025

Removing select() doesn't seem unreasonable. Just give me the go-ahead, and I'll submit a PR.

Good. Could you please proceed to make the change? Thanks a lot!

falbrechtskirchinger added a commit to falbrechtskirchinger/cpp-httplib that referenced this pull request Feb 20, 2025
yhirose pushed a commit that referenced this pull request Feb 20, 2025
* Revert "Fix typo in meson.build (#2070)"

This reverts commit 5c0135f.

* Revert "build(meson): automatically use poll or select as needed (#2067)"

This reverts commit 2b5d1ee.

* Revert "Make poll() the default (#2065)"

This reverts commit 6e73a63.

* Remove select() and use poll()
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