cpr icon indicating copy to clipboard operation
cpr copied to clipboard

Mongoose update

Open saendigPhilip opened this issue 3 years ago • 9 comments

Issue

Solves #757. The webserver should use mongoose 7 instead of mongoose 6, for not using deprecated OpenSSL functions. Furthermore, the test cases simulating slow connections should be fixed.

Tasks

  • [x] Update code, so that it supports the API of mongoose 7
  • [x] Find a way to simulate slow connections
  • [x] Update certificates to make the HTTPS tests work (see #773)
  • [x] Fix failing tests
  • [x] Fix remaining merge conflicts
  • [x] ~~Fix failing windows tests~~

API Changes in mongoose 7

Building

  • Parameter MG_ENABLE_SSL is now MG_ENABLE_OPENSSL

Structural changes

A few functionalities got outsourced to the callback

  • The context (in our case a pointer to an HttpServer or HttpsServer object) is passed as a callback parameter
  • The TLS connection gets initialized inside the callback for each connection, via the mg_tls_init method

Methods -- Renames and changes

  • mg_send_head and mg_printf are now mg_http_reply. For each header, \r\n needs to be appended manually
  • mg_http_send_error does not exist anymore. We replace it with a static SendError method in AbstractServer
  • mg_http_listen is now used for binding and listening instead of mg_bind

Structures -- Renames and changes

  • Trivial renaming of attributes, e.g. pointer attributes formerly named p are now ptr
  • http_message is now mg_http_message. HTTP headers are now stored in an array of struct mg_http_header

Further details

This list of changes is not complete. There are some further minor changes. For further information about mongoose 7, see the mongoose documentation.

Simulation of slow connections

TLDR; It works with mg_timer structs provided by mongoose (also see timer documentation, timer example). For further information, also see the comment below.

saendigPhilip avatar Jun 30 '22 20:06 saendigPhilip

Since I have come up with a solution to simulate slow connections, a few tests are failing. One problem is that if only the header of the response is received, libcurl stores the status code regardless of whether the request succeeded or not. All tests with timeouts expect the response code to be 0, but it is 200, as the header gets transmitted before the body.

My question now is: Is this an issue at the implementation of libcurl, libcpr, or the tests of libcpr?

saendigPhilip avatar Jul 11 '22 07:07 saendigPhilip

In my eyes this is a libcurl bug. Would you like to create an issue for that in libcurl?

COM8 avatar Jul 11 '22 11:07 COM8

Yes, I created an issue here.

saendigPhilip avatar Jul 11 '22 14:07 saendigPhilip

Update: The issue I created at libcurl was marked not-a-bug and closed. The reasoning was that libcurl's behavior reflects the behavior from the documentation, which is that the response code is only 0 if no response was received at all.

My suggestion would be to leave the implementation of libcpr as it is and to remove the failing assertions in the tests. This way we stay consistent with libcurl. To be honest, I agree with the response I got: a user can and should check for specific errors with the error codes provided by libcurl or the response.error attribute in our case.

saendigPhilip avatar Jul 11 '22 19:07 saendigPhilip

Aha, OK. Interesting. Yes, then simply remove the assertion and keep it like that. Please add a note to this issue here inside the code, why we don't assert for the status code here.

COM8 avatar Jul 12 '22 06:07 COM8

Simulating slow connections with mongoose

I finally came up with a way to simulate slow connections with mongoose without nested calls of mg_mgr_poll which lead to a lot of problems. Here is a short summary:

The naive approach

  1. Manually construct an HTTP header inside the mg_printf method
  2. Drain header with mg_mgr_poll
  3. Wait some time
  4. Send the remaining part(s) of the message

The problem with this approach is: We have nested calls of mg_mgr_poll: The response handling method is called from mg_mgr_poll and we call mg_mgr_poll from within the response handling method. This leads to two kinds of issues:

  • The linked list that stores the connections is traversed in a loop. If the list gets updated by nested mg_mgr_poll calls, this confuses the former mg_mgr_poll calls and potentially leads to segfaults and/or memory leaks
  • We stay in the response method of a specific endpoint while handling a request to another endpoint. This makes the two methods interfere with each other

Using mg_timer structs

  • Register timers with (lambda) functions that simply send the message we want to send
    • Set an according timeout
    • As a parameter to the timer, pass a helper structure containing necessary information about the connection
    • All helper structures are stored in a vector of unique_ptrs in the AbstractServer object
  • On each iteration of mg_mgr_poll, all registered timers are called
  • Once a message has been fully sent, the counter is set to a value that doesn't fulfill the sending condition anymore, thus deactivating the timer
  • In the end, all timer arguments are deleted by deleting the aforementioned vector

saendigPhilip avatar Jul 14 '22 12:07 saendigPhilip

Update. Just when I thought my work was finally coming to an end, the tests on windows cause issues...

Windows Port issues

Debugging with Wireshark

As you can see from the CI checks, the tests setting the port of the server as source port are failing. For some reason, binding a socket to the port that is occupied by the server works under windows. This leads to the following network communication: grafik We can see messages sent from port 61936 to exactly the same port 61936. This should definitely not happen.

Curl Request on the test server

This is very unlikely a problem of libcpr, since the same happens when performing the request with curl:

$ curl -v --local-port 61936 127.0.0.1:61936/hello.html
*   Trying 127.0.0.1:61936...
* Local port: 61936
* Connected to 127.0.0.1 (127.0.0.1) port 61936 (#0)
> GET /hello.html HTTP/1.1
> Host: 127.0.0.1:61936
> User-Agent: curl/7.84.0
> Accept: */*
>
* Received HTTP/0.9 when not allowed
* Closing connection 0
curl: (1) Received HTTP/0.9 when not allowed

When performing the same request again, I get:

$ curl -v --local-port 61936 127.0.0.1:61936/not-existing.html
*   Trying 127.0.0.1:61936...
* Local port: 61936
* connect to 127.0.0.1 port 61936 failed: Address already in use
* Failed to connect to 127.0.0.1 port 61936 after 12 ms: Address already in use
* Closing connection 0
curl: (7) Failed to connect to 127.0.0.1 port 61936 after 12 ms: Address already in use

This is still not fully the expected behaviour, since the curl error code is different (for the expected behaviour see below).

Curl Request on a simple Python Server

In my eyes, this seems to be a problem of mongoose under windows, since I have tried performing the requests on a simple python server, which instantly showed the expected behaviour:

$ curl -v --local-port 8080 127.0.0.1:8080/index.html
*   Trying 127.0.0.1:8080...
* bind failed with errno 10048: Address already in use
* Closing connection 0
curl: (45) bind failed with errno 10048: Address already in use

Update

Giving up on this. I tried some simple fixes, and I even tried to write a dedicated test for windows where I create and bind a socket manually and then use this port as a local port. It seemed like this didn't work as well and it even made another test fail. I would simply suggest skipping these tests for windows.

saendigPhilip avatar Jul 24 '22 17:07 saendigPhilip

After I gave up on the tests LocalPortTests.SetLocalPortTestOccupied and LocalPortTests.SetOptionTestOccupied (which I commented out for windows, see previous comment), and solved the remaining merge conflicts, this PR should finally be ready to be reviewed and merged :)

saendigPhilip avatar Jul 26 '22 10:07 saendigPhilip

What is the state of this PR?

COM8 avatar Aug 11 '22 07:08 COM8

Now that I fixed all the merge conflicts again, this PR is again ready to be merged. In one pipeline for macOS MultiperformGetTests.MultiperformTenSessionsGetTest fails, but I have no idea why.

saendigPhilip avatar Aug 12 '22 09:08 saendigPhilip

Ready to go. Thanks for your great work!

COM8 avatar Aug 14 '22 13:08 COM8