Initial attempt at win32 http_listener response refactor

[gigaplex]

For Issue 13: #13

Here is a first cut at a refactor for the response handling to reduce cyclic reference leaks and allow error replies to propagate back to the client. I'm not 100% happy with it, the content_ready_task for example will potentially race against async_process_response if they both attempt to call cancel_request under certain error conditions. There's also no attempt to catch exceptions on the proxy_content_ready completion event, which might cause some unobserved exception issues. I've created the pull request at this early stage to facilitate discussion.

[msftclas]

Hi @gigaplex, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[gigaplex]

CLA is awaiting signature from our legal team, they've informally granted permission at this stage.

[ras0219-msft]

To improve readability, could you use an enum for the bad_request argument of do_response instead of a bool? The meaning of do_response(response::GOOD_REQUEST) is more obvious compared do_response(false).

As mentioned in the other thread, I'm concerned about race conditions regarding the reading and setting of m_msg. If there are possible concurrent accesses, I think every reader will need to "pin" it to the stack by making a local copy of the shared_ptr so that it doesn't get deleted while they are inside a call. Example:

• To start, m_msg is the only strong ref to the actual request object.
• Thread 1 calls m_msg.reply() (link).
• After the pointer load completes on Thread 1, Thread 2 calls m_msg = http_request() (link). This removes the last remaining strong ref to the actual request object, so it is deleted.
• Thread 1 proceeds to modify the now-freed request object, invoking UB of the worst sort.

[gigaplex]

Thanks for the initial feedback. I chose to use a bool instead of an enum because that's how the asio code currently does it, but I agree, I usually prefer enums here.

As for the race condition in your example, m_msg isn't the only strong ref to the request object. There's a local copy in the argument to async_process_request. Also, there's a reason that do_response isn't called until after m_msg.reply() completes, so thread 2 can't call m_msg = http_request() because that callback isn't initialised until after the reply is created. I have however found a race condition of a similar nature in another function, which I'm working to address.

[gigaplex]

I submitted the CLA a few days ago, yet this is still tagged as cla-required. How long does it normally take to process?

[kavyako]

yes, this seems to be taking unusually long, I am following up on this with our CLA automation team.

[msftclas]

Hi @gigaplex, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[gigaplex]

I'm fairly happy with the current state of my changes now. Can I get another round of review for merging please?

One question I do have is that in the m_response_completed callback, immediately after p_context->m_response._get_impl()->_set_server_context(nullptr); there was a call to t.get();. What is the purpose of this call? It wasn't carried over in my refactor, but if it's needed for a specific reason, I can add it back in.

[gigaplex]

I've rebased against 2.7.0.

[ras0219-msft]

Sorry for the delay on this one, thank you very much for your patience and the contribution!