C++ code now uses only R's C API

[pachadotdev]

Dear httpuv maintainers:

I hope you are doing well.

This PR is closely related to r-lib/later#266.

I found a few oddities using this on HM Government infrastructure, the same that led me to edit the 'later' package to have minimal dependencies. In part, co-maintaining the WebTechnologies CRAN task view motivated me to implement these changes.

Instead of replacing Rcpp with cpp11, or cpp4r with is my own mod, I went to a more "bare metal" approach and rewrote later internals to use only R's C API (or Rapi).

I hope this PR can be merged. I installed 'apache' (web server) on my laptop to test all the skipped tests in test-traffic.R.

This PR does not add new dependencies.

Be safe and well,
MVS

[wch]

Hi, I think that the overall goal of removing the dependency on Rcpp is a good one, to reduce the chance of ABI compatibility problems. With a quick look, I don't see any problems that jump out at me. However, I have a number of reservations about taking this PR.

• I assume that this was done with AI coding tools, as it is all in one big commit. I don't have a problem with using AI, because we have been using it extensively for our work and we know what it is capable of. When those tools are used, the kinds of things I want to know are, what questions did the author ask, and what LLM models did they use? I don't plan on reviewing large code changes like this manually, and I would like to know what steps the author took to review it before handing it to us.
• What kind of testing has been done on this? It would be irresponsible for us to accept a big change like this without knowing that there has been extensive testing. It is especially important to test under heavy load, and also test the static file serving to exercise the multithreaded serving code.
• There are places where PROTECT() is called and then it calls a function which could throw an exception. How can we be sure that in all of these places, a balancing UNPROTECT() will be called?
• Going to the plain C API in some ways makes things simpler, but I think it would be better to use cpp11 to provide some abstractions to improve understandability and reduce the risk of mistakes.
• Does this handle RNG state the same way as the original code?

httpuv in its current state is stable and battle tested. With this change, it may be solid, but how can we be sure of that? That is a very important question, because if we accept the change, we become responsible for any problems that arise.

[pachadotdev]

Hi, I think that the overall goal of removing the dependency on Rcpp is a good one, to reduce the chance of ABI compatibility problems. With a quick look, I don't see any problems that jump out at me. However, I have a number of reservations about taking this PR.

* I assume that this was done with AI coding tools, as it is all in one big commit. I don't have a problem with using AI, because we have been using it extensively for our work and we know what it is capable of. When those tools are used, the kinds of things I want to know are, what questions did the author ask, and what LLM models did they use? I don't plan on reviewing large code changes like this manually, and I would like to know what steps the author took to review it before handing it to us.

* What kind of testing has been done on this? It would be irresponsible for us to accept a big change like this without knowing that there has been extensive testing. It is especially important to test under heavy load, and also test the static file serving to exercise the multithreaded serving code.

* There are places where `PROTECT()` is called and then it calls a function which could throw an exception. How can we be sure that in all of these places, a balancing `UNPROTECT()` will be called?

* Going to the plain C API in some ways makes things simpler, but I think it would be better to use cpp11 to provide some abstractions to improve understandability and reduce the risk of mistakes.

* Does this handle RNG state the same way as the original code?

httpuv in its current state is stable and battle tested. With this change, it may be solid, but how can we be sure of that? That is a very important question, because if we accept the change, we become responsible for any problems that arise.

Hi!

I actually edited this by hand over the lapse of several days. Perhaps I should have created another branch with multiple commits? If yes, I can more or less retrace my own mistakes to divide this into multiple commits.

The ABI issue is exactly what I found in the cluster where I run a Shiny dashboard, and so I edited the 'later' package as well (r-lib/later#266 (comment)).

I had cpp11 in mind, then I realised that for 'later' I would need a side package (e.g., 'latertest') and move some unit tests there. Then I thought about the IT side, at least my uni prefers less dependencies. But in principle, this is the same idea you can write with Rcpp, cpp11, or cpp4r (my own thing).

I have a quite simple setup to test, I first wrote an initial draft, then started running devtools::load_all() and devtools::test() until all the original tests (unmodified) passed.

My apologies about the style, I tend to work in my own thesis project and sometimes I use GitHub as some kind of Google Drive and commit all at once once a week (unless there are errors with one of the GHA tests, where I edit more often)

[pachadotdev]

Ps. I can move this to cpp11/cpp4r. Now that I studied all the code for hours and doing lots of tries, it should be easier. I want to simplify my own future as the ABI errors with the cluster are kind of very time consuming, which is why I created https://cpp4r.org/.

[pachadotdev]

also, sorry the ignorance but how do I sign this?

it is not clickable

[wch]

Some thoughts on this PR and r-lib/later#266 and r-lib/later#268.

If you are having ABI incompatibility problems when using these packages, that is a symptom of a problem with how your packages are built and rebuilt when their dependencies are updated. I know that it can be inconvenient to deal with rebuilding downstream dependencies when a package like Rcpp changes, but if you are not doing this, then there is an issue with your system that needs to be addressed. Changing these packages to not use Rcpp will avoid the issue for these packages, but it is still something that you need to make sure is done on your system.

These PRs may help solve the problems you are encountering, but they also have the potential to introduce problems for many other users. You have been looking at this from your perspective, but you need to look at it from the perspective of the maintainers of this software.

The issues here are not just about the code itself, but about larger software engineering issues. These PRs are major changes to the code. This code is pretty stable now, and taking huge changes like this is a big commitment, as is maintaining the code, and dealing with bugs that may arise from it. Changing it to plain C code makes it harder to reason about and harder to write bug-free code. How can we know that the changes are solid? I asked a number of questions that I asked in my previous comment -- what are the answers to those questions?

This PR has changes many comments to refer to your cpp4r package, and those comments are written in a way that endorses cpp4r. And the PR r-lib/later#268 takes a hard dependency on cpp4r. Please look at this from our perspective: cpp4r is a new package which we don't know much about, and we can't be sure that it will be maintained in the future. In contrast, we do know a lot about cpp11, and we know that it will be maintained in the future. This attempt to inject cpp4r into projects where we have not asked for it suggests to me that you don't understand the responsibilities that we have as maintainers of these packages which are stable and widely used.

The changes in r-lib/later#268 appear to make many changes that are not necessary, but perhaps fit your preference, like code formatting, line wrapping, and the order of include statements. Please remember that when you are coming into someone else's project with a PR that they have not asked for, you can't just change everything to your personal preference and expect it to be accepted.

[pachadotdev]

Some thoughts on this PR and r-lib/later#266 and r-lib/later#268.

If you are having ABI incompatibility problems when using these packages, that is a symptom of a problem with how your packages are built and rebuilt when their dependencies are updated. I know that it can be inconvenient to deal with rebuilding downstream dependencies when a package like Rcpp changes, but if you are not doing this, then there is an issue with your system that needs to be addressed. Changing these packages to not use Rcpp will avoid the issue for these packages, but it is still something that you need to make sure is done on your system.

These PRs may help solve the problems you are encountering, but they also have the potential to introduce problems for many other users. You have been looking at this from your perspective, but you need to look at it from the perspective of the maintainers of this software.

The issues here are not just about the code itself, but about larger software engineering issues. These PRs are major changes to the code. This code is pretty stable now, and taking huge changes like this is a big commitment, as is maintaining the code, and dealing with bugs that may arise from it. Changing it to plain C code makes it harder to reason about and harder to write bug-free code. How can we know that the changes are solid? I asked a number of questions that I asked in my previous comment -- what are the answers to those questions?

This PR has changes many comments to refer to your cpp4r package, and those comments are written in a way that endorses cpp4r. And the PR r-lib/later#268 takes a hard dependency on cpp4r. Please look at this from our perspective: cpp4r is a new package which we don't know much about, and we can't be sure that it will be maintained in the future. In contrast, we do know a lot about cpp11, and we know that it will be maintained in the future. This attempt to inject cpp4r into projects where we have not asked for it suggests to me that you don't understand the responsibilities that we have as maintainers of these packages which are stable and widely used.

The changes in r-lib/later#268 appear to make many changes that are not necessary, but perhaps fit your preference, like code formatting, line wrapping, and the order of include statements. Please remember that when you are coming into someone else's project with a PR that they have not asked for, you can't just change everything to your personal preference and expect it to be accepted.

Hi

I get it. In general, the cluster I have been working with has shown ABI issues with packages that I do not maintain.

Changing it to plain C code makes it harder to reason about and harder to write bug-free code. How can we know that the changes are solid? I asked a number of questions that I asked in my previous comment -- what are the answers to those questions?

I think I answered in the other thread or by email. This is why I asked about the value of a cpp11 version for the same. I test with a Makefile that loops a devtools::check() for different GCC/Glang and CXX17/20/23 pairs.

This PR has changes many comments to refer to your cpp4r package, and those comments are written in a way that endorses cpp4r. And the PR r-lib/later#268 takes a hard dependency on cpp4r. Please look at this from our perspective: cpp4r is a new package which we don't know much about, and we can't be sure that it will be maintained in the future.

Not really an endorsement. It is essentially a cpp11 mod with some corrections that I will be actively maintaining at least for two more years.

In contrast, we do know a lot about cpp11, and we know that it will be maintained in the future. This attempt to inject cpp4r into projects where we have not asked for it suggests to me that you don't understand the responsibilities that we have as maintainers of these packages which are stable and widely used.

I'll edit the other PR to use cpp11. I hope that works.

The changes in r-lib/later#268 appear to make many changes that are not necessary, but perhaps fit your preference, like code formatting, line wrapping, and the order of include statements. Please remember that when you are coming into someone else's project with a PR that they have not asked for, you can't just change everything to your personal preference and expect it to be accepted.

That wrapping and others is something I adapted from cpp11 itself. cpp11 uses https://github.com/r-lib/cpp11/blob/main/Makefile#L15, which is something I decided to apply to all my projects to keep a good grammar.

I will comment in the other issue and come back in 24-48 hrs. I am really trying to find a proper solution to my cluster issues that makes IT happy.