Duplicate entries on server response .net

Question

Scenario

One windows service polls a url every two minutes to retrieve certain data.

If any data has been added since the previous call, the data is retrieved and stored otherwise the loop carries on.

Issue

Sometimes a request takes more than two minutes to return a response.

When this happens, the next request is still made and finds new data, since the previous request hasn't return a response yet

This results in duplicate entries when the data is stored.

What I've tried

I tried to handle that by using a boolean like so:

Boolean InProgress = true;
foreach (var item in Lists)
{ 
\\Make a request and return new data (if any)
InProgress = false;
if (InProgress = false)
  {
  \\Store new data
  }
}

This doesn't solve the issue. I believe I'm using the boolean in wrong place, but I'm not sure where it should.

This is the loop that makes the request and store the data

void serviceTimer_Elapsed(object sender, ElapsedEventArgs e)
    {
        try
        {
            Data getCredentials = new Data();
            DataTable credentials = getCredentials.loadCredentials();
            Boolean InProgress = true;

            for (int i = 0; i < credentials.Rows.Count; i++)
            {
                if (credentials != null)
                {
                    var PBranchID = (int)credentials.Rows[i]["PortalBranchID"];
                    var negRef = (int)credentials.Rows[i]["NegotiatorRef"];
                    var Username = credentials.Rows[i]["Username"].ToString();
                    var Password = credentials.Rows[i]["Password"].ToString();
                    var Domain = credentials.Rows[i]["Domain"].ToString();
                    var FooCompanyBaseUrl = "https://" + Domain + ".FooCompany.com/";

                    Data getCalls = new Data();
                    DataTable calls = getCalls.loadCalls(PBranchID);

                    //If it's not the first call
                    if (calls != null && calls.Rows.Count > 0)
                    {
                        //Makes a call
                        DateTime CreatedSince = DateTime.SpecifyKind((DateTime)calls.Rows[0]["LastSuccessOn"], DateTimeKind.Local);
                        string IssueListUrl = FooCompany.WebApi.V2.URLs.Issues(BaseUrl, null, CreatedSince.ToUniversalTime(), null);
                        FooCompany.WebApi.V2.DTO.PrevNextPagedList resultIssueList;

                        resultIssueList = FooCompany.WebApi.Client.Helper.Utils.Getter<Foocompany.WebApi.V2.DTO.PrevNextPagedList>(IssueListUrl, Username, Password);
                        InProgress = false;
                        if (InProgress == false)
                        {
                            if (resultIssueList.Items.Count > 0)
                            {
                                //If call returns new issues, save call
                                Data saveCalls = new Data();
                                saveCalls.saveCalls(PBranchID);

                                foreach (var item in resultIssueList.Items)
                                {
                                    var Issue = FooCompany.WebApi.Client.Helper.Utils.Getter<FooCompany.WebApi.V2.DTO.Issue>(item, Username, Password);

                                    string TenantSurname = Issue.Surname;
                                    string TenantEmail = Issue.EmailAddress;
                                    Data tenants = new Data();
                                    int tenantPropRef = Convert.ToInt32(tenants.loadTenantPropRef(PBranchID, TenantSurname, TenantEmail));
                                    Data Properties = new Data();
                                    DataTable propAddress = Properties.loadPropAddress(PBranchID, tenantPropRef);
                                    var Address1 = propAddress.Rows[0]["Address1"];
                                    var Address2 = propAddress.Rows[0]["Address2"];
                                    var AddressFolder = Address1 + "," + Address2;
                                    if (!Directory.Exists("path"))
                                    {
                                        Directory.CreateDirectory("path");
                                    }
                                    string ReportPDFDestination = "path";

                                    if (File.Exists(ReportPDFDestination))
                                    {
                                        File.Delete(ReportPDFDestination);
                                    }
                                    FooCompany.WebApi.Client.Helper.Utils.DownloadFileAuthenticated(FooCompany.WebApi.V2.URLs.IssueReport(BaseUrl, Issue.Id), Username, Password, ReportPDFDestination);


                                    //Store data 

                                }
                                IssueListUrl = resultIssueList.NextURL;

                            }
                        }

                }
                else
                {
                    continue;
                }
            }

        }


        catch (Exception ex)
        {
            //write to log
        }
    }

Question

I'm sure there is a better way than a boolean.

Could anyone advice a different method to handle the issue properly? Thanks.

Solution

I ended up using a combination of both Thomas and Mason suggestions. I wrapped a lock statement around the main function of my windows service and used a boolean inside the function section that makes the call to the remote server. Tested many times and it's error free.

We need to see your code that calls this code in order to give you a good response. — mason, May 30 '14 at 13:53
I use threads, but I keep track of the state in a flag and I don't allow it to call again until the flag is reset to zero, or exceeds some reasonable time. — Anthony Horne, May 30 '14 at 13:53
You added the code that actually retrieves the data, but that's not really relevant. We need to see what calls the first code you posted. By the way, that first block, you're setting a Boolean to false, then testing to see if it's false right afterwards. It will **always** be false. And you need to use `==` to test for equality. `=` is the assignment operator. — mason, May 30 '14 at 14:02
thanks Mason. I will post the other section of code. Regarding the boolean: it is set to true when I declare it. — U r s u s, May 30 '14 at 14:05
@Mason did you have a chance to check the new section I posted? — U r s u s, May 30 '14 at 14:30
Yes, and it suffers from the *exact same problem I mentioned before*. You set InProgress to false, and directly afterwards you check to see if it's false. Of course it's false. It will always be false when you get to that if statement since you're setting it to false right before you get to it. — mason, May 30 '14 at 14:39
I feel I'm missing something but that's what I want: I want the code inside the if statement to be executed only when inprogress is false. — U r s u s, May 30 '14 at 14:43
Would it solve my problem if I set the boolean to true at the end of the loop? — U r s u s, May 30 '14 at 14:44

Thomas · Accepted Answer · 2014-05-30T15:11:23.383

2

You seems to have a problem of synchronisation, just surround the code that iterate though the List with a lock, and you will be fine.

public class MyClass{
private readonly object internalLock= new object();
private bool AlreadyRunning { get;  set; }
void serviceTimer_Elapsed(object sender, ElapsedEventArgs e)
{
    if(AlreadyRunning){
        return;
    }

    try{
        lock(internalLock){     
            Thread.MemoryBarrier();

            if(AlreadyRunning){
                return;
            }
            AlreadyRunning = true;

            ...Do all the things...
        }
    }
    catch(Exception e){
        ..Exception handling
    }
    finally
    {
        AlreadyRunning = false;
    }
}

edited May 30 '14 at 15:11

answered May 30 '14 at 13:51

Thomas

5,603
5
32
48

The line that makes the request needs to be inside the loop. So i'm not sure about locking the code inside it. – U r s u s May 30 '14 at 14:01
@dura You need to give us more code so. You seems to have a synchronisation issue. Now, you can try to find it yourself or give us more code! – Thomas May 30 '14 at 14:04
Sorry I was having a nightmare to delete the sensitive info from the code. But I've added it now. – U r s u s May 30 '14 at 14:19
In the meantime I will try the lock solution. – U r s u s May 30 '14 at 14:19
Can you confirm that your problem is that you have several `ReportPDFDestination` generated for the same Issue? – Thomas May 30 '14 at 14:31
That's correct. I don't however get two files for the same issue because it overwrites the old one if it's the same. – U r s u s May 30 '14 at 14:35
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/54803/discussion-between-thomas-and-dura). – Thomas May 30 '14 at 14:36
I'm still not sure about the code you posted. I've read this http://stackoverflow.com/questions/2005211/when-to-use-lock-vs-memorybarrier-in-net and it looks like using a memory barrier as well as a lock is not a good idea. – U r s u s May 30 '14 at 19:59

mason · Answer 2 · 2014-05-30T14:47:45.983

2

bool InProgress=false;

void serviceTimer_Elapsed(object sender, ElapsedEventArgs e)
    {
    if(!InProgress)
        {
        InProgress=true;
        //retrieve data
        InProgress=false;
        }
    }

Your InProgress variable needs to be declared outside the event handler. When you enter the method, check to see if it's already running. If it is, then we do nothing. If it's not running, then we say it's running, retrieve our data, then reset our flag to say we've finished running.

You'll probably need to add appropriate locks for thread safety, similar to Thomas's answer.

edited May 30 '14 at 14:47

answered May 30 '14 at 14:42

mason

31,774
10
77
121

thanks that's very clear. I'll wait to mark my answer to see which one I end up using. – U r s u s May 30 '14 at 14:59
2

Carefull,if an exception is thrown, then you will stop retieving data as `InProgress` will stay `false`. a try/catch/finally is important I think. – Thomas May 30 '14 at 15:08
1

@Mason thanks very much for your time. I've marked Thomas' as the answer because he took some extra time on the chat to advise me (and he also has less votes than you) – U r s u s Jun 01 '14 at 21:38