1

The following C++ code fragment makes use of Microsoft's C++ Rest SDK. I'm not understanding why the first fragment works and other's don't. I'm assuming the differences are due to object destruction and scoping rules. I'm looking for an explanation of why the first fragment works and the other fragments hang on close(). Also, what can the SDK do to eliminate future mistakes. Some really smart people looked at the fragment but never saw the issue.

First code fragment. This fragment works and is shown in its entirety. Subsequent code fragments replace the marked code within. Please focus on the differences and not other distractions. The code was tested by making a single GET request in a browser and stepping through the code. In all cases, request.reply() was executed once and only once.

    boost::lockfree::spsc_queue<web::http::http_request, boost::lockfree::capacity<1024>> queue;
web::http::experimental::listener::http_listener listener(U("http://localhost:3901"));
listener.support([&](web::http::http_request request)
{
    queue.push(request);
});
listener.open().wait();
std::cout << "listening ... hit enter to initiate shutdown." << std::endl;
std::getchar();
// BEGIN CODE IN QUESTION
while (!queue.empty())
{
    web::http::http_request request;
    if (queue.pop(request))
        request.reply(web::http::status_codes::ServiceUnavailable, U("Service Unavailable")).wait();
}
// END CODE IN QUESTION
listener.close().wait();

Second code fragment. Hangs on close().

    // hangs on close().wait()
web::http::http_request request;
while (queue.try_pop(request))
{
    request.reply(web::http::status_codes::ServiceUnavailable, U("Service Unavailable")).wait();
}

Third code fragment. Hangs on close().

    // hangs on close().wait(). Outer braces make no difference.
 {
    web::http::http_request request;
    while (queue.try_pop(request))
    {
        request.reply(web::http::status_codes::ServiceUnavailable, U("Service Unavailable")).wait();
        request.~http_request();
    }
}

Forth code fragment. Hangs on close(). Outer braces make no difference.

    // hangs on close().wait()
{
    web::http::http_request request;
    while (queue.try_pop(request))
    {
        request.reply(web::http::status_codes::ServiceUnavailable, U("Service Unavailable")).wait();
        request.~http_request();
    }
    request.~http_request();
}

Update: Supporting Matt McNabb's explanation, the following code works if I only issue one GET. I simply removed the loop to process the single GET. The explicit destructor call is required to avoid a hang but it's incorrect practice.

    web::http::http_request request;
requests.pop(request);
request.reply(web::http::status_codes::ServiceUnavailable, U("Service Unavailable")).wait();
    request.~http_request();

Update: An explicit destructor call after the loop makes the program work for a single GET. However, two or more GETs throw an exception. I'm unsure why.

    web::http::http_request request;
while (queue.try_pop(request))
{
    request.reply(web::http::status_codes::ServiceUnavailable, U("Service Unavailable")).wait();
}
request.~http_request();
BSalita
  • 8,420
  • 10
  • 51
  • 68
  • 2
    All of the lines `request.~http_request();` cause undefined behaviour and should be removed. (Technically the first one doesn't, but then UB comes on the second one, or when the function exits). It is undefined behaviour to call a destructor twice for the same object, or use the object after the destructor has been called. – M.M Jun 21 '14 at 12:59
  • The difference between 1 and 2 is that 1 destroys the old request and creates a new one each time; but 2 re-uses the same object without destroying or creating it each loop iteration. It would be useful if you post the definition of `try_pop` so we can see by what means it updates the existing object. Also it may be that `web::http::http_request` is not designed to support whatever update method `try_pop` used. – M.M Jun 21 '14 at 13:04
  • This might be a silly question, but what is `try_pop`? It doesn't appear in [boost 1.55's interface documentation of `spsc_queue`](http://www.boost.org/doc/libs/1_53_0/doc/html/boost/lockfree/spsc_queue.html) – dyp Jun 21 '14 at 13:04
  • concurrent_queue::try_pop() returns *false* when the queue is empty. So you are sending a response when there was no request. Surely this should be `while (queue.try_pop(request))`, no ! operator. – Hans Passant Jun 21 '14 at 13:06
  • Guys, thanks for the comments. Indeed I did mix spsc_queue, concurrent_queue and the ! operator. I'll edit out the ! operator. try_pop() consumes the front of queue returning the item as the argument. – BSalita Jun 21 '14 at 13:19
  • So, what is it then? Is it `concurrent_queue` or a `spsc_queue`? Did you have the `!` in your original code? – dyp Jun 21 '14 at 13:21
  • Sorry for the confusion. The ! operator is not in the code. All fragments correctly executes one and only one request.reply(). – BSalita Jun 21 '14 at 13:24
  • Sorry, I changed queue namespaces and should have mentioned that. They illustrate the scope issue with their different method calls. concurrent_queue doesn't have empty() and spsc_queue doesn't have try_pop(). In the 2nd and subsequent fragments, the code hangs. – BSalita Jun 21 '14 at 13:27
  • Does the first snippet use a `spsc_queue` and the second one a `concurrent_queue`? – dyp Jun 21 '14 at 13:27
  • Correct, the first snippet uses spsc_queue and the subsequent use concurrent_queue. The issue isn't the method call or the namespace, it's the object destruction. – BSalita Jun 21 '14 at 13:28
  • assuming `try_pop` calls the assignment operator to "return" the popped value; possibly `web::http::http_request request` is not designed to handle this occurrence. The `reply` function says that it asynchronously replies;but perhaps then the assignment operator overwrites stateful things about the object that is currently in the process of replying. – M.M Jun 21 '14 at 13:29
  • to be clear, the assignment operator does not call the destructor and constructor (although it's possible that it is implemented by calling a cleanup function which is also called by the destructor). [the documentation](http://msdn.microsoft.com/en-us/library/jj969505.aspx) says nothing about what the assignment operator does. It'd be important to know what happens to shared resources (e.g. a socket handle) in this case – M.M Jun 21 '14 at 13:30
  • 2
    re. your update mentioning my name, get rid of `request.~http_request();`. It causes undefined behaviour. "C++'s object destruction rules" include that you are only allowed to call a destructor once. – M.M Jun 21 '14 at 13:35
  • @MattMcNabb Regarding the assigment operator, is there a typical practice or best practice for cleanup? – BSalita Jun 21 '14 at 13:38
  • Note that the 2nd and subsequent fragments, the code in enclosed in braces. That was my attempt to make request go out of scope. Apparently, that didn't work. Why is that? – BSalita Jun 21 '14 at 13:41
  • Not specifically, although it's expected that objects have proper value semantics. It could be considered a bug if the assignment operator has not been disabled, but using it causes either the previous or the new request to misbehave. – M.M Jun 21 '14 at 13:41
  • "Apparently, that didn't work." - in fragment 2, `request` lives for the whole duration of the `while` `try_pop` loop, and is destroyed at the closing brace of that fragment. (The outer loop probably only has one iteration, if `try_pop` keeps returning `true until the queue is empty). But if our theory about the assignment operator is true, then just one instance of it returning true twice in a row is enough to cause the problem. In 3 and 4, it is UB due to the manual destructor calls. – M.M Jun 21 '14 at 13:44
  • You could try `while(true) { http_request request; if(!queue.try_pop(request)){break;} request.reply(..).wait(); }` to see if it has something to do with reuse. – dyp Jun 21 '14 at 13:56
  • I have to retract my statement that braces not forcing request to go out of scope. In fact braces makes the close().wait() not hang so I'm assuming the difference is that the braces makes request go out of scope. I'm now testing the other examples and will remove the outer braces to avoid confusion. The main issue of destruction and scope still stands. – BSalita Jun 21 '14 at 13:58
  • After further testing, a set of outer braces makes the second fragment work but not any of the others. So for the second example, braces make request drop out of scope. For the third and fourth example, the issue is try_pop(request) in the while loop doesn't correctly destroy the request object and any attempts to call ~http_request() can't fix the issue. – BSalita Jun 21 '14 at 14:14
  • 2
    Let me repeat that as clear as possible: **If you call the destructor explicitly of an object on the stack, you will get Undefined Behaviour since the destructor will additionally get called a second time (automatically)**. Do not do that. Rather, focus on *why* the second example does not work, and try to construct a new request object on each loop iteration, to see if that resolves the problem. – dyp Jun 21 '14 at 14:46
  • First attempt to summarize. The devil is in using try_pop(response) or similar bool returning semantics in a while loop. The reason is the response object's scope is difficult to control. The solution is to use the first code fragment listed. You can also use the second code fragment enclosed within braces but that looks silly. The reason try_pop() returns a bool has to do with concurrency but it makes the programmer tempted to use it in a loop. Maybe there's a questionable implementation of the assignment operator, it can avoid this issue by cleaning up the current contents of the object(?). – BSalita Jun 21 '14 at 14:49
  • anyway the try_pop seems concurrency-safe (http://msdn.microsoft.com/en-us/library/ee355368.aspx) – Marco A. Jun 21 '14 at 14:58
  • The issue which caused the original post has been resolved in the C++ Rest Sdk development branch of 1-Jul-2014. – BSalita Jul 01 '14 at 21:42

2 Answers2

2

The problem with each of these examples appears to be the same: mistmatched construction and destruction of the request object. I have rarely seen such ingenuity in doing it wrong.

The simple solution is:

  1. Declare the object inside a block, at which point it will be allocated.
  2. Do not call the destructor explicitly (rarely a good idea).
  3. The object will be destroyed when the block exits.

It would seem that it needs to be be destroyed before the call to close(), which is reasonable.

By which I mean that the second example is the only one in which the object is not destroyed at all, and it too fails. I assume this is for some other reason not visible in this code.

david.pfx
  • 10,520
  • 3
  • 30
  • 63
  • Doesn't the second example imply, because it causes a hang, that the response object's copy or assignment methods aren't cleaning up the destination object? Is this what's going wrong? – BSalita Jun 21 '14 at 15:05
  • Different question, not enough info to speculate. For this Q&A just make sure you destroy the object exactly once, preferably by block exit and not an explicit dtor call. – david.pfx Jun 21 '14 at 15:13
1

Disclaimer: Without having any knowledge on that library, this is rather dangerous

#include <iostream>
using namespace std;

class Obj {
public:
    Obj() {
        cout << "Constructor" << endl;
    }
    ~Obj() {
        cout << "Destructor" << endl;
    }
};

int main() {
    {
        Obj ea;
        ea.~Obj();
    }
    return 0;
}

Output:

Constructor 
Destructor 
Destructor

If there are resources to be freed or operations to be performed to clean up the request, you're causing a lot of troubles in every case except when you're performing the following steps:

  1. Getting the request by copy (pop or try_pop)
  2. Processing the request
  3. Destructing and cleaning up the request ONCE

In the fragments above:

  1. The object is copied from the queue, processed and destroyed. Each one of them. Fine.
  2. The object is copied, processed but not destroyed. Hangs.
  3. The object is copied, processed, destroyed and then destroyed again. Hangs.
  4. Even worse than 3.
  5. The object is copied, processed and destroyed. Fine for the listener, it could trigger problems at the end of the function's scope.
  6. The object is copied, processed and only one of those is getting the destructor called for the cleanup - thus only one request. Fine for one request.
Marco A.
  • 43,032
  • 26
  • 132
  • 246
  • 1
    Regarding 2, yes, the back-queued request object is not destroyed. The program will hang unless the response object goes out of scope. – BSalita Jun 21 '14 at 15:02
  • The issue which caused the original post has been resolved in the C++ Rest Sdk development branch of 1-Jul-2014. – BSalita Jul 01 '14 at 21:42