TLS tickets and FreeRTOS workers#52
Conversation
6f8061b to
714601c
Compare
fhessel
left a comment
There was a problem hiding this comment.
Sorry it took so long, but this was quite a bit of code, and as you've merged the develop branch into your feature branch, GitHub displays those things as incoming changes, too. I hope I didn't mix things up that aren't relevant for this PR.
Many of the remarks are just styling issues, some of them are related to file descriptors (where 0 is actually valid), and some points may need a bit of discussion. I'd suggest to address those individually on the commented lines.
What I still need/want to do before merging is some performance testing in a more controlled environment (e.g. with a Python script to compare it the current master). The results for the example page in the browser already look great, but I want to play around a bit with client pool sizes, multiple clients and see how the server reacts in absence of Content-Length headers. The latter is only important until #15 gets implemented, I think.
And just so you know: I've a local copy of your branch that is already prepared for merging into develop, so you do not need to take care of the merge conflicts. I'll just wait before pushing it as that makes it easier for you to add more commits to the PR.
There was a problem hiding this comment.
This should be image/jpeg
| res->setHeader("Content-Type", "image/jpg"); | |
| res->setHeader("Content-Type", "image/jpeg"); |
| req->discardRequestBody(); | ||
|
|
||
| // Find the file in SPIFFS | ||
| String filename = String(req->getRequestString().c_str()); |
There was a problem hiding this comment.
I'd maybe use std::string filename = req->getRequestString() here and filename.c_str() when passing it as parameter. I'm aware of the confusion with Arduino's String and the standard C++ std::string, but the rest of the code doesn't use the Arduino variant, either, to allow better portability.
| res->setHeader("Content-Type", "image/jpg"); | ||
| // Informational only, if you look at developer console in your browser | ||
| char taskHandle[11]; | ||
| sprintf(taskHandle, "0x%08x", (uint32_t)xTaskGetCurrentTaskHandle()); |
There was a problem hiding this comment.
I'd be happier with a snprintf(taskHandle, sizeof(taskHandle), ... if we can assume C++ 11
There was a problem hiding this comment.
| sprintf(taskHandle, "0x%08x", (uint32_t)xTaskGetCurrentTaskHandle()); | |
| snprintf(taskHandle, sizeof(taskHandle), "%p", (uint32_t)xTaskGetCurrentTaskHandle()); |
| _running = false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I see that it's handled by HTTPServer::stop(), but I think a destructor that waits for run() to terminate if _running==true would be a good idea, as run() accesses class members of this worker instance and the task is otherwise decoupled from the instance. So, at the moment, it depends on the user of the class to assure that the task has finished before calling delete on the worker.
Or at least something like this, to identify wrong usage:
| HTTPWorker::~HTTPWorker() { | |
| if (_running) { | |
| HTTPS_LOGE("Deleting worker while task is running!") | |
| } | |
| } | |
| // Create the instance flagged as running | ||
| // If constructor fails to start the actual task, | ||
| // or task has ended due to server shutdown, this will be set to false. | ||
| bool _running = true; |
There was a problem hiding this comment.
This means the worker is actually _running before start() is called.
|
|
||
| void setContentLength(size_t size); | ||
| bool isResponseBuffered(); | ||
| bool correctContentLength(); |
There was a problem hiding this comment.
I'd prefer is as a prefix to methods returning booleans which start with a verb. When I first read that method name, I thought it would correct the length, in the sense of fix or adjust.
(If you apply this suggestion, also apply the ones in HTTPResponse.cpp and HTTPConnection.cpp)
| bool correctContentLength(); | |
| bool isCorrectContentLength(); |
| res.setHeader("Connection", "keep-alive"); | ||
| res.finalize(); | ||
| } | ||
| if (res.correctContentLength()) { |
There was a problem hiding this comment.
(see HTTPResponse.hpp)
| if (res.correctContentLength()) { | |
| if (res.isCorrectContentLength()) { |
| } | ||
| } | ||
| } | ||
| _sentBytesCount += size; |
There was a problem hiding this comment.
This should not be += size but += whatever writeBytesInternal() returns.
| } else if (size > 0) { | ||
| // Try buffering | ||
| if (_responseCacheSize > 0) { | ||
| _responseCache = new byte[_responseCacheSize]; |
There was a problem hiding this comment.
Failure of allocation is not handled.
| virtual void initialize(int serverSocketID, HTTPHeaders *defaultHeaders); | ||
| virtual int initialAccept(); | ||
| virtual int fullyAccept(); |
There was a problem hiding this comment.
To me, it's not clear how this separation between accepting only the raw socket or performing the complete TLS handshake provides an advantage for checking if there is data.
HTTP Case:
accept()runs the regular three-way-handshake, and the server is then able to check whether the connection is idling or not byselecting the socket or trying to read.
→ No advantage of separation
HTTPS Case:
accept()does only the three-way-handshake. In this case, there will always be data on the line, which is theClientHellorecord of the TLS connection.SSL_accept()removes the TLS handshake and the wrapping of the application data, so we need to call that function to see if there's application data on the line
→ We need both before we can know if the client actually uses this connection.
And if it's about the memory overhead of the pending TLS connection, I'd just drop the idling connection before accepting the new one.

Two new API functions added:
HTTPSServer->enableTLSTickets(uint32_t liftimeSeconds = 86400, bool useHardwareRNG = false)
Which enables HTTPS server generating and accepting RFC5077 TLS tickets from clients for session negotiation vs. complete renegotiation for each TLS connection
HTTPServer->enableWorkers(uint8_t numWorkers = 2, size_t taskStackSize = HTTPS_CONN_TASK_STACK_SIZE, int taskPriority = HTTPS_CONN_TASK_PRIORITY);
Which creates n-FreeRTOS tasks that will takeover processing of server's loop() method. More than one task will enable parallel processing of connections as ESP32 has two cores and/or if single resourceNode's handler function is busy doing some longer processing