First Draft Mutexing. Testing Now Needed. #55
Conversation
| @@ -21,10 +21,11 @@ RequestHandler::Status StatusHandler::HandleRequest(const Request& request, | |||
|
|
|||
| ServerStatus::Status status = status_->GetStatus(); | |||
|
|
|||
|
|
|||
| status.stsMx->lock(); | |||
There was a problem hiding this comment.
Why are you locking the status here? You lock it and then don't use the resource.
| @@ -21,10 +21,11 @@ RequestHandler::Status StatusHandler::HandleRequest(const Request& request, | |||
|
|
|||
| ServerStatus::Status status = status_->GetStatus(); | |||
|
|
|||
|
|
|||
| status.stsMx->lock(); | |||
| response->SetStatus(Response::OK); | |||
| response->AddHeader("Content-Type", http::mime_type::ContentTypeAsString(http::mime_type::CONTENT_TYPE_TEXT_HTML)); | |||
| response->SetBody(StatusToHtml(status)); | |||
There was a problem hiding this comment.
In fact, you should lock it right above here.
There was a problem hiding this comment.
Yes I see that now, moved
| @@ -33,15 +34,17 @@ RequestHandler::Status StatusHandler::InitStatus(ServerStatus* status) { | |||
| if (status == nullptr) { | |||
| return RequestHandler::MISCONFIGURED_HANDLER; | |||
| } | |||
| status->GetStatus().stsMx->lock(); | |||
| status_ = status; | |||
There was a problem hiding this comment.
This call on the copy constructor is not safe -- you're copying the mutex, for example
There was a problem hiding this comment.
What is the best way to remedy this?
There was a problem hiding this comment.
Sorry, this was my mistake
| @@ -92,8 +92,30 @@ void Session::do_read() { | |||
| status_->LogIncomingRequest(req->uri(), 400); | |||
| } | |||
|
|
|||
| do_write(response_string); | |||
| delete response; | |||
| if(RequestStatus == RequestHandler::BAD_REQUEST) { | |||
There was a problem hiding this comment.
How does this code relate to the code above it?
Also tabbing is wrong.
There was a problem hiding this comment.
It was a copy paste error, fixed.
| @@ -134,5 +134,6 @@ bool WebServer::run_server() { | |||
| printf("Exception %s\n", e.what()); | |||
| return false; | |||
| } | |||
| status_.GetStatus().destroyMutex(); | |||
There was a problem hiding this comment.
Sounds like this should be part of the default constructor
There was a problem hiding this comment.
Do you mean destructor? The problem with that is that it was being called multiple times on one instance, producing segfaults. Likely related to copy error above.
There was a problem hiding this comment.
You can check if a object is valid (i.e. it still exists) before destructing it.
Either way the struct should never be copied by value
| std::map<std::string, int> RequestCountByURL_; | ||
| std::map<int, int> ResponseCountByCode_; | ||
| std::map<std::string, std::string> RequestHandlers_; | ||
|
|
||
| std::mutex* stsMx; |
There was a problem hiding this comment.
This needs a underscore aftewards to indicate data internals.
|
@avikbmohan when are you getting back to this...? |
No description provided.