3

I get lots of ERROR_INTERNET_CANNOT_CONNECT (12029 code) in callback procedure of the request. I use WinHttp in async mode(on a server). How do you cleanly close the connection in this case. Do you just use something like this(like you normally close a connection?):

            ::WinHttpSetStatusCallback(handle, NULL, 0, 0);
            ::WinHttpCloseHandle(this->handle));

I ask this because I have some strange memory leaking associated with winhttp dll that occurs in the situation described(want to create hundreds of concurrent connections that are probably blocked by the firm internal firewall or destination server drops the connections). I have already looked at the documentation of WinHttpCloseHandle on msdn...

Here is how I do handling of the callback states:

    template <typename T>
void WinHttp::AsyncRequest<T>::OnCallback(DWORD code, const void* info, DWORD length)
{
    T* pT = static_cast<T*>(this);

    switch (code)
    {
    case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE:
    case WINHTTP_CALLBACK_STATUS_WRITE_COMPLETE:
        {
            HRESULT result = pT->OnWriteData();
            if (FAILED(result))
            {
                throw CommunicationException(::GetLastError());
            }
            if (S_FALSE == result)
            {
                if (!::WinHttpReceiveResponse(handle, 0)) // reserved
                {
                    throw CommunicationException(::GetLastError());
                }
            }
            break;
        }

    case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE:
        {
            DWORD statusCode;
            DWORD statusCodeSize = sizeof(DWORD);
            if (!::WinHttpQueryHeaders(handle, WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER, WINHTTP_HEADER_NAME_BY_INDEX, &statusCode, &statusCodeSize, WINHTTP_NO_HEADER_INDEX))
            {
                throw CommunicationException(::GetLastError());
            }
            pT->OnStatusCodeReceived(statusCode);
            if (!::WinHttpQueryDataAvailable(handle, 0))
            {
                // If a synchronous error occured, throw error.  Otherwise
                // the query is successful or asynchronous.
                throw CommunicationException(::GetLastError());
            }

            break;
        }

    case WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE:
        {
            unsigned int size = *((LPDWORD) info);
            if (size == 0)
            {
                pT->OnResponseComplete(S_OK);
            }
            else
            {
                unsigned int sizeToRead = (size <= chunkSize) ? size : chunkSize;
                if (!::WinHttpReadData(handle, &buffer[0], sizeToRead, 0)) // async result
                {
                    throw CommunicationException(::GetLastError());
                }
            }
            break;
        }

    case WINHTTP_CALLBACK_STATUS_READ_COMPLETE:
        {
            if (length != 0)
            {
                pT->OnReadComplete(&buffer[0], length);
                if (!::WinHttpQueryDataAvailable(handle, 0))
                {
                    // If a synchronous error occured, throw error.  Otherwise
                    // the query is successful or asynchronous.
                    throw CommunicationException(::GetLastError());
                }
            }
            else
            {
                pT->OnResponseComplete(S_OK);
            }

            break;
        }

    case WINHTTP_CALLBACK_STATUS_REQUEST_ERROR:
        {
            {
                throw CommunicationException(::GetLastError());
            }

            break;
        }
    }
}

Here buffer is a vector that has reserved 8K once the request is initiated. Thanks in advance.

In OnResponseComplete, OnResponsEerror I eventually call also:

::WinHttpSetStatusCallback(handle, NULL, 0, 0);
 assert(::WinHttpCloseHandle(this->handle));
 this->handle = nullptr;
Ghita
  • 4,465
  • 4
  • 42
  • 69
  • 1
    So, are you closing handles right from the callback? – Roman R. Jul 23 '12 at 10:36
  • Basically yes. They are indirectly called as a result of a class destruction (class WinHttp::AsyncRequest calls delete on itself, and is allocated on the head because of that) – Ghita Jul 23 '12 at 10:55
  • I don't suffer from handle leaks. Thyy are not shown as growing. – Ghita Jul 23 '12 at 10:57
  • 1
    Memory leak is not the only problem. You are closing the handle while obviously you have the handle is use higher on the call stack. We all expect WinHTTP to handle this nicely, but it is in your power to do things nicer on your side as well. Why don't you put the handle onto a queue/list and then close the outstanding handles somewhere outside of the callback, not being in the middle of things. – Roman R. Jul 23 '12 at 11:11
  • @RomanR. That is another idea to try. If that is required it does complicate design indeed. – Ghita Jul 23 '12 at 12:10
  • I'm doing ::WinHttpCloseHandle from another context without luck. What I find interesting is that in Debug mode I don't see this memory leak... – Ghita Jul 23 '12 at 12:54
  • What I've found out is that in WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE I was reading only the status code. Added to read all headers with WinHttpQueryHeaders->WINHTTP_QUERY_RAW_HEADERS_CRLF even if I don't use them but still a leak. – Ghita Jul 23 '12 at 13:36

2 Answers2

3

Roman R was right about the issue just want to include more details in the response. Cancellation is tricky. For WinHTTP you need to remember that callbacks fire on a thread pool. They can arrive on a random thread and since the thread pool does not guarantee when the callback is executed, you could call WinHttpSetStatusCallback to clear the callback and then later on still receive callbacks. You thus need to synchronize this yourself. A more subtle problem is that you cannot call back into WinHTTP from a callback. The safest and most reliable way to handle WinHTTP callbacks is to dispatch the callbacks to a single thread. This could be a UI thread or a single-threaded thread pool. You then also need to close the handle first and then wait for the callback to indicate that the handle is closed (WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING) – only then is it safe to release any connection-specific resources, callbacks, etc.

My tests so far showed that closing the request handle must be done outside the callbacks and also the final closing of the connection handle. One can queue up the intent to close the request and afterwards the connection closing and other cleanup on a separate thread using QueueUserApc so that at least those 2 operations are synchronized.

Ghita
  • 4,465
  • 4
  • 42
  • 69
  • 5
    Might want to acknowledge the source of your answer given that you copy-pasted the explanation I sent you via email. :) – Kenny Kerr Jul 26 '12 at 17:07
  • @KennyKerr Yes Kenny you were the source. I didn't know you had a stack account. If you take a look here http://social.msdn.microsoft.com/Forums/en-US/casablanca/thread/546cebc1-7082-4dce-a436-50ec00105e04 you will understand the reason. Copying paste was the easiest way :-) – Ghita Jul 26 '12 at 19:52
  • @KennyKerr In case I get an error while calling WinHttp API in async mode (or simply get WINHTTP_CALLBACK_STATUS_REQUEST_ERROR) is it possible/safe all the time to close the request by calling ::WinHttpCloseHandle so that I get WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING event in status callback ? I ask this because I rely on WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING event coming all the time for connection/request cleanup – Ghita Jul 31 '12 at 14:19
0

EDIT The problem still remains. What I wrote below did not fix the issue.

The problem described was actually not very much related to ERROR_CANNOT_CONNECT but to the way I was "incorectly" handling the async status callback notifications of winhttp. If anyone would be interested I will copy here a typical way of handling the status notifications.

Ghita
  • 4,465
  • 4
  • 42
  • 69
  • After more testing I came to the conclusion that I still leak memory. I will add in the question description how I do handling of callbacks state. – Ghita Jul 23 '12 at 10:10
  • Found out more about how handling of winhttp callbacks should take place (thanks to a man that was prompt on helping me). Once I'll have a proof of concept I'll write that down. – Ghita Jul 24 '12 at 20:06