-2

i have code which use asio coroutine to get file using http get asynchronously.
now i try to modify this code to get 24 files ,i want to to make container which has boost::futures"not std::futures" so that i can use wait_for_any to handle ready to use future.
i want to convert my coroutine to something which keeps asynchronous http get ,and in the same time returns future of file name when http get finishes"correct or with error".
this is my coroutine:

   void HTTPRequest::Execute(boost::asio::yield_context yield_r, std::string request_name, boost::shared_ptr<std::map<std::string, boost::shared_ptr<HTTPResponse>>> mHTTPClient_Responses_Map_shared_pointer)
//4-9-2020 trial of promise
//13std::string HTTPRequest::Execute(boost::asio::yield_context yield_r, boost::promise<std::string> &p, std::string request_name, boost::shared_ptr<std::map<std::string, boost::shared_ptr<HTTPResponse>>> mHTTPClient_Responses_Map_shared_pointer)
{
    std::map<std::string, boost::shared_ptr<HTTPResponse>> & mHTTPClient_Responses_Map = boost::ref(*mHTTPClient_Responses_Map_shared_pointer).get() ;
    ptime startFetch = second_clock::local_time();
    boost::unique_lock<boost::mutex> cancel_lock(mCancelMutex);
    if (mWasCancelled)
    {
        cancel_lock.unlock();
        OnFinish(boost::system::error_code(boost::asio::error::operation_aborted));

        m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " has been cancelled by the user at start of HTTPRequest::Execute coroutine." << std::endl;
        m_formatting_ostream.flush();
        ////allam2020 change UniqueSignalValue to url
        boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "cancelExecute", request_name, m_formatting_ostream_string);
        m_formatting_ostream.clear();

        //p.set_value(request_name);
        //return request_name;

    }
    cancel_lock.unlock();

    bool iterator_failed = true;////    allam 2020 where is this variable changed?????????? is it before SendRequest()??????? after async_connect retruns successfully
    boost::system::error_code ec;
    for (auto iterator_resolve : *mRequestSharedPtrVecResolverIterator)
    {
        BOOST_LOG((*mHTTPRequest_LoggingInstance_shared_pointer).mloggerCoutLog) << "Request #" << this->GetId() << " for " << mUrl <<" trying to send request using " << iterator_resolve->endpoint().address().to_string() << std::endl;

        // Compose the request message.
        mRequestBuf += "GET " + mUri + " HTTP/1.1\r\n";
        // Add mandatory header.
        mRequestBuf += "Host: " + mHost + "\r\n";
        mRequestBuf += "\r\n";

        for (int mIrange : boost::irange(0, ATTEMPTS))
        {
            HTTPRequest::mIrange = mIrange;
            ////allam2020 1111111111111111111111111111111111111111111111111111111111
            resolver_iterator iterator_connect = boost::asio::async_connect(mSock, iterator_resolve, yield_r[ec]);////allam 2020 this gets us back to io_stream_run

            if (ec.value() == boost::system::errc::errc_t::success)//(ec.value()==0)
            {               
                ////allam 2020
                iterator_failed = false;//????????????/////???????

                boost::unique_lock<boost::mutex> cancel_lock(mCancelMutex);
                if (mWasCancelled)
                {
                    cancel_lock.unlock();
                    OnFinish(boost::system::error_code(boost::asio::error::operation_aborted));

                    m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " has been cancelled by the user after returning from async_connect inside HTTPRequest::Execute using"<< iterator_resolve->endpoint().address().to_string() << std::endl;
                    m_formatting_ostream.flush();
                    boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "cancel_async_connect_Execute", iterator_resolve->endpoint().address().to_string(),m_formatting_ostream_string);
                    m_formatting_ostream.clear();

                    //p.set_value(request_name);
                    //return request_name;
                }

                cancel_lock.unlock();

                // Send the request message.
                SendRequest(yield_r);////alllam 2020 VVVVVVVVIIIIIIIIIIIIIPPPPPPPPPPP MILESTONE
            }
            else if (ec.value() != boost::system::errc::errc_t::success)//(ec.value()==0) //(ec.value() != 0)
            {
                OnFinish(ec);               
                BOOST_LOG((*mHTTPRequest_LoggingInstance_shared_pointer).mloggerCoutLog) <<"Request #" << this->GetId() << " for " << mUrl <<" failed after trying " << mIrange << "times" << " to async_connect inside HTTPRequest::Execute " << std::endl;                
                continue;
            }


            ////allam 2020  now test for mContinue_for ,this is done for other functions recurdively called inside SendRequest
            if (mContinue_for==true)
            {
                mContinue_for = !(mContinue_for);
                boost::this_thread::sleep_for(boost::chrono::seconds(mIrange));

                continue;
            }

            // Response is correct.
            //log str_status_code
            //Logger.info("Fetched {0} completed in {1}s".format(id, time.time() - start))
            //allam2020
            m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << "Fetched " << mUrl << " completed in : " << (second_clock::local_time() - startFetch) << "with HTTP code :" << mResponsePtr->get_status_code() << "\n" << "and the code reasonPhrase is :" << HttpStatus::reasonPhrase(static_cast<int>(mResponsePtr->get_status_code())) << "with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
            m_formatting_ostream.flush();

            boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "http_request_completed", HttpStatus::reasonPhrase(static_cast<int>(mResponsePtr->get_status_code())), m_formatting_ostream_string);
            m_formatting_ostream.clear();

            //if len(buffer.getbuffer()) <= 0:
            if (mResponsePtr->get_response_buf().size() <= 0)
            {
                //Logger.info("Buffer for {0} is empty ".format(id))
                //allam2020
                m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << "Fetched " << mUrl << " with Buffer for " << mUrl << " is empty  " << "\n" << "with HTTP code :" << mResponsePtr->get_status_code() << "\n" << "and the code reasonPhrase is :" << HttpStatus::reasonPhrase(static_cast<int>(mResponsePtr->get_status_code())) << std::endl;
                m_formatting_ostream.flush();

                boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "http_request_completed_empty", HttpStatus::reasonPhrase(static_cast<int>(mResponsePtr->get_status_code())), m_formatting_ostream_string);
                m_formatting_ostream.clear();

            }

            //continue work on response
            ////std::string response_name = "response_" + request_name;////allam 2020 make this member variable????????????//4-22-2020 yes 
            mHTTPRequest_response_name = "response_" + request_name;
            mHTTPClient_Responses_Map[mHTTPRequest_response_name] = GetResponseSharedPtr();

            break;
        }

        //the following conditions test the result of send request
        if (mSendRequest == 0)
        {
            if (mReadStatusLine == 0)
            {
                if (mHttp_1_1 == 0)
                {
                    if (mStatusCodeNot200 == 0)
                    {
                        if (mReadResponseHeaders == 0)
                        {
                            if (mReadResponseBody == 0)
                            {
                                ////allam2020 4-4-2020 no error present and response is recieved in its mHTTPResponse SO DO NOTHING 
                            }
                            else if (mReadResponseBody != 0)
                            {
                                m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " has failed completely after trying" << ATTEMPTS << "times" << " to async_read inside HTTPRequest::ReadResponseBody to get ResponseBody  " << "with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
                                m_formatting_ostream.flush();
                                boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_async_read_inside_HTTPRequest_ReadResponseBody", "requestFailed_ReadResponseBody_Iterator_ " + iterator_resolve->endpoint().address().to_string(),m_formatting_ostream_string);
                                m_formatting_ostream.clear();
                            }
                        }
                        else if (mReadResponseHeaders != 0)
                        {
                            m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " has failed completely after trying" << ATTEMPTS << "times" << " to async_read_until inside HTTPRequest::ReadResponseHeadersto get ResponseHeaders " << "with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
                            m_formatting_ostream.flush();
                            boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_async_read_until_inside_HTTPRequest_ReadResponseHeaders", "requestFailed_ReadResponseHeaders_Iterator_ " + iterator_resolve->endpoint().address().to_string(), m_formatting_ostream_string);
                            m_formatting_ostream.clear();
                        }
                    }
                    else if (mStatusCodeNot200 != 0)
                    {
                        m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " has failed completely after" << ATTEMPTS << "times" << " to async_read_until inside HTTPRequest::ReadStatusLine because of status_code not 200:" << http_errors::invalid_response << "the error code is :" << mStatusCode << "\n" << "and the error reasonPhrase is :" << HttpStatus::reasonPhrase(static_cast<int>(std::stoul(mStatusCode))) << "with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
                        m_formatting_ostream.flush();
                        boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_StatusCodeNot200_inside_HTTPRequest_ReadStatusLine", "requestFailed_ReadStatusLine_StatusCodeNot200:" + mStatusCode + "_Iterator_ " + iterator_resolve->endpoint().address().to_string(), m_formatting_ostream_string);
                        m_formatting_ostream.clear();
                    }
                }
                else if (mHttp_1_1 != 0)
                {
                    ////4-2-2020
                    m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " after trying " << ATTEMPTS << "times" << " to async_read_until inside HTTPRequest::ReadStatusLine because of bad not http/1.1 version response" << mHTTP_Version << "recieved with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
                    m_formatting_ostream.flush();
                    boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_Http_1_1_inside_HTTPRequest_ReadStatusLine", "requestFailed_ReadStatusLine_Http_1_1_Iterator " + iterator_resolve->endpoint().address().to_string(), m_formatting_ostream_string);
                    m_formatting_ostream.clear();
                }
            }
            else if (mReadStatusLine != 0)
            {
                ////4-2-2020
                m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " after trying " << ATTEMPTS << "times" << " to async_read_until inside HTTPRequest::ReadStatusLine  with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
                m_formatting_ostream.flush();
                boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_async_read_until_inside_HTTPRequest_ReadStatusLine", "requestFailed_ReadStatusLine_Iterator " + iterator_resolve->endpoint().address().to_string(), m_formatting_ostream_string);
                m_formatting_ostream.clear();
            }
        }
        else if (mSendRequest != 0)
        {
            ////4-2-2020
            m_formatting_ostream << "Request #" << this->GetId() << " for " << mUrl << " after trying " << ATTEMPTS << "times" << " to async_write inside HTTPRequest::SendRequest  with certain resolver iterator " << iterator_resolve->endpoint().address().to_string() << std::endl;
            m_formatting_ostream.flush();
            boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_async_write_inside_HTTPRequest_SendRequest", "requestFailed_SendRequest_Iterator " + iterator_resolve->endpoint().address().to_string(), m_formatting_ostream_string);
            m_formatting_ostream.clear();
        }



        if (iterator_failed == true)
        {
            m_formatting_ostream << "Request failed for " << mUrl << " after trying " << ATTEMPTS << "times" << " to async_connect inside HTTPRequest::Execute with certain resolver iterator "<< iterator_resolve->endpoint().address().to_string() << std::endl;
            m_formatting_ostream.flush();
            ////allam 2020 i might need to pass iterator resolve which has failed
            boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_async_connect_inside_HTTPRequest_Execute", "requestFailed_Iterator " + iterator_resolve->endpoint().address().to_string(), m_formatting_ostream_string);
            m_formatting_ostream.clear();


            continue;////allam 2020 here i should continue for next iterator
        }
    }
    if (iterator_failed == true)
    {
        m_formatting_ostream << "Request failed for " << mUrl << " after trying " << ATTEMPTS << "times" << " to async_connect inside HTTPRequest::Execute with ALL resolver iterators" << std::endl;
        m_formatting_ostream.flush();
        ////allam 2020 i might need to pass iterator resolve which has failed
        boost_log_function(mHTTPRequest_LoggingInstance_shared_pointer, "failed_async_connect_inside_HTTPRequest_Execute", "requestFailed_Iterator" + GetmUrlLogger(),m_formatting_ostream_string);////allam2020 ?????i might need to change this from GetmUrlLogger to request name argument of Execute???????????????????4-2-2020
        m_formatting_ostream.clear();
        ////allam 2020 here i should return from execute because no resolved address could be used so the whole execute request operation failed

        //p.set_value(request_name);
        //return request_name;
    }
    ////allam2020 should i put if conditions for mSendRequest ....mReadResponseBody????? to identify final complete error at these functions and end of 5 attempts

}

there are other functions SendRequest ... which are called from Execute but i didnot put them to make less code.
if required i will post them.

sehe
  • 374,641
  • 47
  • 450
  • 633
ahmed allam
  • 377
  • 2
  • 15

1 Answers1

1

It's not often I run out of breath just reading the code. This is one of those times. You should probably just state your goal.

  • Also, mRequestSharedPtrVecResolverIterator suggests you are storing a vector of resolver results, only to iterate through them and try to do the request.

    Did you know that you can just use boost::asio::[async_]connect to do that for you?. It looks like a simple boost::asio::async_connect should solve all your problems with the resolver iterators and

  • you should just return the response instead of magically setting it into the map (you don't need to have access to other entries).
  • Finally you can cut 99% of the shared pointers and that will save you more time than the contrived optimization with m_formatting_ostream which I imagined as

    std::string m_formatting_ostream_string;
    boost::iostreams::stream<boost::iostreams::back_insert_device<std::string>>
        m_formatting_ostream{ m_formatting_ostream_string };
    

    but you keep repeating error-prone and inefficient code like

    m_formatting_ostream << ... << std::endl;
    m_formatting_ostream.flush(); // this is redundant already
    boost_log_function(
        ...,
        m_formatting_ostream_string);
    m_formatting_ostream.clear();
    
  • There's a lot more confused code. E.g. what is going on here?

    boost::ref(*mHTTPClient_Responses_Map_shared_pointer).get();

boost::ref(*p).get() is by definition just *p.

  • Everything is stateful and worse: the state is mutated. For example,

    mRequestBuf += "GET " + mUri + " HTTP/1.1\r\n";
    // Add mandatory header.
    mRequestBuf += "Host: " + mHost + "\r\n";
    mRequestBuf += "\r\n";
    

    happens each time through the loop, but it never gets reset. It should probably not happen multiple times (that's wasteful)

  • boost::this_thread::sleep_for(boost::chrono::seconds(mIrange)); is an enormous red flag in async IO. You should be awaiting results without sleeping.

  • You're using an entire mutex around a boolean flag. Just make that atomic

    std::atomic_bool mWasCancelled{ false };
    

    Also, using unique_lock makes no sense if you do the unlocking manually 100% of the time.

  • In general there is a tendency for very verbose (Hungarian) names and ditto comments. This probably tries to get a sense control, but the opposite is happening: the code becomes so "impressive" and "accounted" that it is actually unclear what it should do and why it should work.

  • This Christmas Tree anti pattern is what exceptions were invented for. That also removes the need for Execute to "know" about every little implementation detail of SendRequest and the many inscrutable state flags it can set to mysterious values.

enter image description here

Okay. I'm really out of breath. I won't be able to fix this, or even recognize the question (because there wasn't a single future in sight). Instead, I'd suggest working of something like https://www.boost.org/doc/libs/develop/libs/beast/example/http/client/async/http_client_async.cpp for a slightly less heavy-handed approach that will lend itself for maintainance.

sehe
  • 374,641
  • 47
  • 450
  • 633