Non-blocking, non-threaded HTTP client implementation

Question

This is a C# WinForms program targeting .Net 4.5. I am using it for a movie organizer application that will contact sites like OMDB, MyApiFilms, etc. for data about the movies. At the main application level a new movie list needs to have its movies matched to their info, which means searching these sites via their API, identifying the correct search result, downloading that additional details, downloading poster images from IMDB, and so on. The behavior that I'm trying to accomplish with the http downloader portion of the code is that I want to support multiple connections at once, I want each one to report back to the main application as soon as it is done without blocking to wait for the other requests to finish, and ideally I want it to report back via an event so that the downloader is simple to use from the main application.

Originally I implemented this with asynchronous delegates and BeginInvoke/EndInvoke and Invoke to get back on the UI thread to fire an event, but reading through SO and Stephen Cleary's blog I learned that this is bad because it blocks thread pool threads for every connection and is not scalable or efficient for that reason. This is my next attempt and I would be grateful if any problems I don't see could be pointed out to me. The googling I did mostly turned up suggestions to await all after sending a batch, which was not what I wanted because I don't want 9 good requests waiting on 1 that is going to eventually time out (I have really poor internet at home and long timeouts are necessary) and I also don't want those connection slots held up - meaning I am trying to be kind to the site by not hammering it with too many concurrent requests, but if 9 have returned I can send 9 more immediately while waiting on the 1 slow or failed response.

Here's the call at the main application and the event handler: EDIT: Added full event handler code per request.

private void SearchMovie(Movie movie, SearchTerms terms)
{
    //Do some other application stuff and then send http request to OMDBapi.com
    OMDBSearcher osearch = new OMDBSearcher();
    osearch.ResponseArrived += OMDBSearcher_OnResponseArrived;
    osearch.Search(terms, movie);
}

private void OMDBSearcher_OnResponseArrived(object sender, OMDBSearchDoneEventArgs e)
{
    object o = e.Caller;
    //check for errors
    if (e.Error!=null)
    {
        if (o is Movie)
        {
            Movie m = (Movie)o;
            m.SearchFailed = true;
            m.ActiveSearch = false;
            PrecacheMovies.Remove(m);
        }
        DisplayErrorResults(e.Error);
        return;
    }
    //No errors, deal with result
    if (o is Movie)
    {
        Movie m = (Movie)o;
        m.SearchResults = e.Results;
        if (e.Results == null || e.Results.Count == 0)
        {
            m.SearchFailed = true;
            m.NoResults = true;
            Notify("No results returned for " + e.Terms.ToString());
        }
        m.ActiveSearch = false;
        foreach (OMDBMovieStub stub in e.Results)
            GetDetails(stub);//Secondary search for more info about each search result.  Still implemented with async delegates right now.
        if (m == Terms.SearchedMovie)
        {//Terms is a form object that gets set during a user-initiated search (rather than background search)
            DisplaySearchResults(e.Results);
        }
        PrecacheMovies.Remove(m); //PrecacheMovies is part of a temporary throttling mechanism
    }
}

This is similar in some ways to working with a BackgroundWorker but seems less annoying.

OMDBSearcher is a request-type specific object that is a wrapper around a generic http request processor called HttpGetter. The interface OMDBSearcher uses to access HttpGetter is similar to what the main application uses to access OMDBSearcher: call synchronous method, receive response by event. In addition to the event handler it must provide a delegate for processing the HttpWebResponse into the desired result format T. Each type of request (OMDBSearch, OMDBGetDetails, MyApiFilmsSearch, DownloadImage, etc.) would have its own wrapper class around HttpGetter to handle the type specific details.

class OMDBSearcher
{
    private List<OMDBMovieStub> GetSearchResultsFromResponse(HttpWebResponse response)
    {// delegate to process the response into type T.  In this case a List of OMDBMovieStub objects, each representing 1 of up to 10 search results}

    private String GetSearchURL(SearchTerms terms){//request type specific formatting of the request url}

    private void RequestDone(object sender, ResponseEventArgs<List<OMDBMovieStub>> e)
    {
        //this event handler just re-packages and re-sends the EventArgs from the HttpGetter event

        OMDBSearchDoneEventArgs E=new OMDBSearchDoneEventArgs(
            e.Result,(SearchTerms)e.State,e.Caller,e.Error);
        EventHandler<OMDBSearchDoneEventArgs> handler = ResponseArrived;
        if (handler != null) handler(this, E);
    }

    public EventHandler<OMDBSearchDoneEventArgs> ResponseArrived;

    public void Search(SearchTerms terms, object caller)
    {
        HttpGetter<List<OMDBMovieStub>> getter = new HttpGetter<List<OMDBMovieStub>>(
            new ResponseProcessorDelegate<List<OMDBMovieStub>>(GetSearchResultsFromResponse)
            );
        getter.ResponseArrived += RequestDone;
        String requestURL = GetSearchURL(terms);
        getter.Request(new RequestInfo(requestURL, terms, caller));  
        //terms is being sent via the state object
    }
}
public class OMDBSearchDoneEventArgs
{
    public OMDBSearchDoneEventArgs(List<OMDBMovieStub> results, SearchTerms terms, object caller, Exception ex) {//constructor}
    public Exception Error { get; set; }
    public object Caller { get; set; }
    public List<OMDBMovieStub> Results { get; set; }
    public SearchTerms Terms { get; set; }
}

And here is the generic class where the async is happening:

class HttpGetter<T>
{
    public HttpGetter(ResponseProcessorDelegate<T> responseProcessor)
    {
        ResponseProcessor = responseProcessor;
        //default values if not set
        this.ContentType = "text/xml; encoding='utf-8'";
        this.Timeout = 60000;
    }
    public EventHandler<ResponseEventArgs<T>> ResponseArrived;
    public String ContentType { get; set; }
    public int Timeout { get; set; }
    public async void Request(RequestInfo reqInfo)
    { 
        try
        { 
            //Set up http request
            HttpWebRequest myHttpWebRequest = (HttpWebRequest)HttpWebRequest.Create(reqInfo.URL);
            myHttpWebRequest.Method = "GET";
            myHttpWebRequest.ContentType = this.ContentType;
            myHttpWebRequest.Timeout = this.Timeout;
            //Async await
            WebResponse webResponse= await myHttpWebRequest.GetResponseAsync();
            //Process response and compose event args
            HttpWebResponse myHttpWebResponse = (HttpWebResponse)webResponse;
            T result = ResponseProcessor(myHttpWebResponse);
            ResponseEventArgs<T> e = new ResponseEventArgs<T>(result, reqInfo.State, reqInfo.Caller, null);
            Respond(e);

        }
        catch (Exception ex)
        //exceptions must be passed from async void via the event args, Background Worker style.
        {
            ResponseEventArgs<T> e = new ResponseEventArgs<T>(default(T),reqInfo.State, reqInfo.Caller, ex);
            Respond(e);
        }
    }

    private void Respond(ResponseEventArgs<T> e)
    {
        EventHandler<ResponseEventArgs<T>> handler = ResponseArrived;
        if (handler != null) handler(this, e);
    }

    private ResponseProcessorDelegate<T> ResponseProcessor;
}

delegate T ResponseProcessorDelegate<T>(HttpWebResponse response);

public class ResponseEventArgs<T>:EventArgs
{
    public ResponseEventArgs(T result, object state, object caller, Exception ex)
    {//constructor}
    public Exception Error { get; set; }
    public object State { get; set; }
    public object Caller { get; set; }
    public T Result { get; set; }
}
public class RequestInfo
{
    public RequestInfo(String url, object state, object caller)
    {//constructor}
    public String URL {get;set;}
    public object Caller {get;set;}
    public object State { get; set; }
}

I realize from reading Stephen Cleary's blog that async void is not best practice because of exceptions not being thrown, but I hope I have dealt with that objection sufficiently. It seemed the only way to avoid blocking with an await at some point, and without the await I wouldn't receive those exceptions through normal paths anyway.

I subbed this OMDBSearcher object out for my old delegate.BeginInvoke threadpool code for those searches, and it seems to work well (and on one thread!), but I'm pretty new to async/multi-threaded code and would like to make sure that my basic approach is correct before I begin building a connection scheduler thread around it to manage the demand I'm placing on the API servers, building the other request wrappers, and so on. Is what I'm doing ok? Could it be better or simpler?

Answer 1

Have you considered using HttpClient class from .Net 4.5+? That makes your code lot cleaner.

Also why not take Async pattern all the way? From your initiator (button click or main etc) you can call a async method, which will call all the downstream methods. That way it will be more scalable (as framework will take care of worker threads, scheduling etc)and will not block your main thread.

Currently you call the Request from main thread, but don't wait for the it to complete. You are adding the whole event mechanism to pass back the response. Instead you can make Request return a Task<ResponseArgs>(ResponseArgs is nothing but ResponseEventArgs but named to match the pattern)

Then your code can be much easy to read/understand/maintain

public async void Search(SearchTerms terms, object caller)
{
    HttpGetter<List<OMDBMovieStub>> getter = new HttpGetter<List<OMDBMovieStub>>(
        new ResponseProcessorDelegate<List<OMDBMovieStub>>(GetSearchResultsFromResponse)
        );
    String requestURL = GetSearchURL(terms);
    var response = await getter.Request(new RequestInfo(requestURL, terms, caller));  
    //process the response, what you are currently doing in event handler
}

Ideally your SearchTerms should return a task as well. Any exceptions from the Request should be handled in within the SearchTerms and it can return just a bool to indicate a success or failure.

Where you are calling SearchTerms (which in itself will be marked as async you will have

var tasks = Movies.Select(t=>Search(t));
await Task.WhenAll(tasks);

async Task Search(Movie m) 
{
    SearchTerms terms=GetSearchTerms(m); 
    await Search(terms,m);
}

This way you will line up all the work items (tasks) once, and let the system take care of executing them. Once all tasks are complete your code will proceed. Makes sense?

Answer 2

Here is how I implemented Adarsha's answer: The application method now initiates the search and processes the results when they return.

private async void SearchMovieAsync(Movie movie,SearchTerms terms)
{
    try  //must catch errors here, they will not propagate up
    {
        //Initiate search
        OMDBGetter getter=new OMDBGetter();
        List<OMDBMovieStub> results = await getter.Search(terms);
        //Application logic on results
        if (results == null || results.Count == 0)
        {
            movie.SearchFailed = true;
            movie.NoResults = true;
            Notify("No results returned for " + terms.ToString());
        }
        movie.ActiveSearch = false;
        foreach (OMDBMovieStub stub in results)
            GetDetails(stub);
        if (movie == Terms.SearchedMovie) DisplaySearchResults(results);
        PrecacheMovies.Remove(movie); //ignore if not in list
    }
    catch (Exception ex)
    {
        movie.SearchFailed = true;
        movie.ActiveSearch = false;
        PrecacheMovies.Remove(movie);
        DisplayErrorResults(ex);
    }
}

There is also an overload of SearchMovie(Movie movie) that creates default search terms out of the movie info, but the 2 parameter version is required for user specified search terms. HttpGetter has been removed. That code is too simple to need an object. OMDBGetter now combines the functionality that was in HttpGetter and OMDBSearcher

class OMDBGetter
{
    public OMDBGetter()
    {
        this.Timeout = 60000;
    }
    public int Timeout { get; set; }
    private String GetSearchURL(SearchTerms terms)
    {
        //convert terms object to search url
    }
    private async Task<List<OMDBMovieStub>>   GetSearchResultsFromResponse(HttpResponseMessage response)
    {
        System.IO.Stream stream = await response.Content.ReadAsStreamAsync();
        //Convert stream to object list via XPath
    }
    private async Task<HttpResponseMessage> Request(String requestURL)
    {
        HttpClient client = new HttpClient();
        HttpResponseMessage response = await client.GetAsync(requestURL);
        response.EnsureSuccessStatusCode();  //generate exception if required
        return response;
    }
    public async Task<List<OMDBMovieStub>> Search(String searchURL)
    {//This method makes it possible for a consumer of the object to  compose their own search URL
        HttpResponseMessage r = await Request(searchURL);
        List<OMDBMovieStub> result = await GetSearchResultsFromResponse(r);
        return result;
    }
    public async Task<List<OMDBMovieStub>> Search(SearchTerms terms)
    {
        return await Search(GetSearchURL(terms));
    }
}

The connections are processed asynchronously and independently of one another. No waiting for a whole batch to finish. This is much cleaner, thank you for the help!