1

I'm using a windows service for call API, get response and update in Sql Table It's working fine but some time it's Hit API Twice. I'm unable to got reason behind. Here is my code

protected override void OnStart(string[] args)
{
  this.timer = new System.Timers.Timer(15000D);
  this.timer.AutoReset = true;
  this.timer.Elapsed += new System.Timers.ElapsedEventHandler(this.timer_Elapsed);
  this.timer.Start();
}
protected override void OnStop()
  {
     this.timer.Stop();
     this.timer = null;
 }
protected void timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
{
  this.proccessQue();
}

And here is proccessQue() method

//SELECT record form table 
SqlDataAdapter adap = new SqlDataAdapter("SELECT * FROM TABLE_NAME WHERE is_done=0 AND date>DATEADD(minute,-5,GETDATE())", conn);
DataTable dt = new DataTable();
adap.Fill(dt);
for (int i = 0; i < dt.Rows.Count; i++)
{
  string parameters= dt.Rows[i]["parameters"] + "";
  string api = "http://domain.com/page.aspx?parameters=" + parameters;
  HttpWebRequest httpreq = (HttpWebRequest)WebRequest.Create(api);
  HttpWebResponse httpres = (HttpWebResponse)httpreq.GetResponse();
  StreamReader sr = new StreamReader(httpres.GetResponseStream());
  string results = sr.ReadToEnd();
  sr.Close();
  if (results.Contains("<?xml version=\"1.0\" encoding=\"utf-8\" ?>"))
  {
     try
     {
      string response= "";
      XmlDocument xmlDoc = new XmlDocument();
      xmlDoc.LoadXml(results);
      var res2 = xmlDoc.SelectNodes("RechargeRequest/RequestResponse/APIRef");
      if (res2 != null)
        response= res2[0].InnerText;
      SqlCommand cmd = new SqlCommand("UPDATE TABLE_NAME SET field='" + response+ "',is_done=1 WHERE id=" + rId, conn);
      conn.Open();
      cmd.ExecuteNonQuery();
      conn.Close();
    }
    catch (Exception ex)
    {

    }
  }
}

Please help me where is I am going wrong.

Amit Mishra
  • 285
  • 2
  • 5
  • 17
  • Multiple things 1. Your API call is in a loop of records. So will be hit 0-n times based on the results of your loop. It could be your query returns more than one method. 2. Is it possible your processQueue method takes longer than each elapsed event of the timer? My suggestion would be to 1. Check the result of your query as it may be returning more than 1 record 2. Stop your timer in the elapsed event process the queue and then restart it. Finally given this is 2017 your better off using an async/await pattern and use the more robust HttpClient class for web requests. – Nico Feb 28 '17 at 06:59
  • Ohh and finally you have quite a few IDisposable's not being disposed – Nico Feb 28 '17 at 07:00
  • Yes it is possible Query is return more then 1 row. How to Stop timer in the elapsed event process the queue and then restart it? please help me. I'm using this service for recharge website so can not increase timer elapsed time due to recharge time. – Amit Mishra Feb 28 '17 at 07:03
  • In the timer_Elapsed event call timer.Stop(). Then call the processQueue method then call timer.Start(). This will ensure the timer is only waiting fo r 15seconds when the process queue is finished. – Nico Feb 28 '17 at 07:07
  • You mean protected void timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e) {this.timer.Stop(); this.proccessQue(); } like that? – Amit Mishra Feb 28 '17 at 07:10
  • Yes. That will stop the timer immediately. Then right after the this.processQue(); restart it with this.timer.Start() – Nico Feb 28 '17 at 07:11

1 Answers1

1

Based on my comments to the original question there are a few things to look at.

  1. The API will be hit 0-n times based on the results of the query. Now the timer will execute the timer_Elapsed() method asynchronously for every interval. Therefore if the processQue() method takes longer than 15 seconds that the API may be called multiple times for each item.

Therefore one option is to Stop the timer execute your process logic and the Start() the timer when the processQue() method is finished. Such as:

protected void timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
{
  this.timer.Stop(); //stop the timer 
  this.proccessQue(); //process the queue
  this.timer.Start(); //restart the timer
}

So this ensures the processQue(); finishes before the timer_Elapsed() event is called again.

Now in the event of an exception in processQue() method the execution will not continue. Its up to you how to handle this but a simple try .. catch will handle the exceptions (not properly but will).

Now my second concern with the code, and this is not related to why the execution runs multiple times, is the use of the classes and not disposing things correctly.

Firstly.. Why use a SqlDataAdapter when a SqlDataReader will produce a faster execution. This is opinion based, however doesnt require a DataTable and does read the entire result into memory. ho It appears you are only using two columns (not sure where rId comes from) of the Sql Query, so dont use *, but define the column names you actually need. This will reduce the amount of data queried and streamed from the Sql Query. May seem trivial in small queries but can make a big difference in bigger queries and larger data sets.

Next issue I see is the use of IDisposables without disposing of them.

  1. SqlDataAdapter
  2. StreamReader
  3. SqlCommand

These are all classes that inherit from IDisposable so should be wrapped in a using statement or disposed of manually calling the Dispose() method.

Community
  • 1
  • 1
Nico
  • 12,493
  • 5
  • 42
  • 62