1

I need to do some WebRequest to a certain endpoint every 2 seconds. I tried to do it with a Timer, the problem is that every call to the callback function is creating a different Thread and I'm havind some concurrence problems. So I decided to change my implementation and I was thinking about using a background worker with a sleep of two seconds inside or using async await but I don't see the advantages of using async await. Any advice? thank you.

This is the code that I will reimplement.

private void InitTimer()
{
    TimerCallback callback = TimerCallbackFunction;
    m_timer = new Timer(callback, null, 0, m_interval);
}

private void TimerCallbackFunction(Object info)
{
    Thread.CurrentThread.Name = "Requester thread ";
    m_object = GetMyObject();
}

public MyObject GetMyObject()
{
    MyObject myobject = new MyObject();

    try
    {
        MemoryStream responseInMemory = CreateWebRequest(m_host, ENDPOINT);
        XmlSerializer xmlSerializer = new XmlSerializer(typeof(MyObject));
        myObject = (MyObject) xmlSerializer.Deserialize(responseInMemory);
    }
    catch (InvalidOperationException ex)
    {
        m_logger.WriteError("Error getting MyObject: ", ex);
        throw new XmlException();
    }
    return myObject;
}

private MemoryStream CreateWebRequest(string host, string endpoint)
{
    WebRequest request = WebRequest.Create(host + endpoint);
    using (var response = request.GetResponse())
    {
        return (MemoryStream) response.GetResponseStream();
    }
}

EDIT: I have read this SO thread Async/await vs BackgroundWorker

acostela
  • 2,597
  • 3
  • 33
  • 50
  • avoid using shared variables in your callback to avoid concurrency issues. Replace `m_object = GetMyObject();` with something else like `PrintheadsStatus m_object = GetMyObject();` – Khanh TO Feb 10 '16 at 10:56
  • Thanks, that's a good point but the main problem that I'm having is that the response from the webRequest for almost all requests is null. I think that it's because you can not access more than twice at the same time to the same endpoint... – acostela Feb 10 '16 at 11:01
  • 1
    What concurrency problems? Throwing an unnecessary (and obsolete) BackgroundWorker isn't going to fix problems if you don't know what the problem is. Is the request taking too long? – Panagiotis Kanavos Feb 10 '16 at 11:02
  • The problem is when I do more than twice requests at the same time, the response I get is null. Should I use a lock to avoid more than once request at the same time? – acostela Feb 10 '16 at 11:03
  • Because you are using Callbacks which goes out of the main thread, you have to wait for the response using ManualResetEvent for some time, Until the response comes back. – Bewar Salah Feb 10 '16 at 11:06
  • You can't have *two* concurrent requests when you use *one* timer. What do you mean the response is null? That m_Object is null? Has the timer even started? Did an exception occur in your timer event that was lost? You don't show any other code that could set it to null. – Panagiotis Kanavos Feb 10 '16 at 11:07
  • @BewarSalah there's no callback in this code – Panagiotis Kanavos Feb 10 '16 at 11:08
  • @PanagiotisKanavos here is the callback. m_timer = new Timer(callback, null, 0, m_interval); – Bewar Salah Feb 10 '16 at 11:08
  • I have two concurrents calls because Timer is creating a new Thread every two seconds – acostela Feb 10 '16 at 11:09
  • If you want to stop until the previous loop finishes. you can stop the timer then start it. – Bewar Salah Feb 10 '16 at 11:10
  • Do you want the concurrent calls or not? In any case, it seems your requests take far too long. You can also set the timer to fire 2 seconds *after* the last callback finishes, not every 2 seconds – Panagiotis Kanavos Feb 10 '16 at 11:11
  • @PanagiotisKanavos it is required to pool for the server every 2 seconds, I tried it with the Timer and because of that I reached the concurrency problem. I searched to do the same with a single Thread and I read about backgorundworker but after a deeper research I found that is obsolete and It's better using async await but I didn't see the advantages because I will have multithreading again and the same problem. That's the reason of my question. Thank you – acostela Feb 10 '16 at 11:14

2 Answers2

2

async await is also concurrence. If you have concurrence problems and you want your application to have only one thread, you should avoid using async await.

However the best way to do WebRequest is to use async await, which does not block the main UI thread.

Use the bellow method, it will not block anything and it is recommended by Microsoft. https://msdn.microsoft.com/en-us/library/86wf6409(v=vs.110).aspx

private async  Task<MemoryStream> CreateWebRequest(string host, string endpoint)
{
    WebRequest request = WebRequest.Create(host + endpoint);
    using (var response = await request.GetResponseAsync())
    {
        return (MemoryStream)response.GetResponseStream();
    }
}
Bewar Salah
  • 567
  • 1
  • 5
  • 15
  • Thanks but this application won't have UI it will be a library, so the request won't block any UI. But thank you for your quick response. So if I want only one thread is better backgroundworker? – acostela Feb 10 '16 at 10:59
  • But when you use the dll inside another application, it will block it especially when you have a big mount of data to download. – Bewar Salah Feb 10 '16 at 11:00
  • Sorry but what is request.GetResponseAsync ? Is a function included in WebRequest? because I can't find it – acostela Feb 10 '16 at 11:16
  • Yes it is there. what is your target framework? It should be 4.5 and above – Bewar Salah Feb 10 '16 at 11:33
1

You don't mention what the concurrency problems are. It may be that the request takes so long that the next one starts before the previous one finishes. It could also be that the callback replaces the value in my_Object while readers are accessing it.

You can easily make a request every X seconds, asynchronously and without blocking, by using Task.Delay, eg:

ConcurrentQueue<MyObject> m_Responses=new ConcurrentQueue<MyObject>();
public async Task MyPollMethod(int interval)
{
    while(...)
    {
        var result=await SomeAsyncCall();
        m_Responses.Enqueue(result);
        await Task.Delay(interval);
    }
}

This will result in a polling call X seconds after the last one finishes.

It also avoids concurrency issues by storing the result in a concurrent queue instead of replacing the old value, perhaps while someone else was reading int.

Consumers of MyObject would call Dequeue to retrieve MyObject instances in the order they were received.

You could use the ConcurrentQueue to fix the current code too:

private void TimerCallbackFunction(Object info)
{
    Thread.CurrentThread.Name = "Requester thread ";
    var result=GetMyObject(); 
    m_Responses.Enqueue(result);
}

or

private async void TimerCallbackFunction(Object info)
{
    Thread.CurrentThread.Name = "Requester thread ";
    var result=await GetMyObjectAsync(); 
    m_Responses.Enqueue(result);
}

if you want to change your GetObject method to work asynchronously.

Since your request seems to take a long time, it's a good idea to make it asynchronous and avoid blocking the timer's ThreadPool thread while waiting for a network response.

Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236
  • Thank you very much for your complete response, this has clear so many doubts that I had. I will try it now and change my design following your knowledge. Thanks! – acostela Feb 10 '16 at 11:29