0

I want to create a user for a program. If the user allready exists i dont want to create the user with that username.

I got 2 threads Thread 1: handles a socket connection Thread 2: handles a SQL connection

Thread 1 sends USER INFO (name, PhoneNumber and so on) to Thread 2. I want Thread 2 to to notify Thread 1 if the USER allready exists.

Adding the USER INFO to the SQL database, is no problem. I just need that notification.

public void saveUser(string fullName, string CPR, string password, string kontakt)
       {

           ADBconn.Open();

           SqlCommand cmd = new SqlCommand("INSERT INTO ADBregister (fuldeNavn, password, borger_cprnr, kontaktPersonNummer)" + "VALUES (@fuldeNavn, @password, @borger_cprnr, @kontaktPersonNummer)", ADBconn);

           cmd.Parameters.AddWithValue("@fuldeNavn", fullName);
           cmd.Parameters.AddWithValue("@password", password);
           cmd.Parameters.AddWithValue("@borger_cprnr", CPR);
           cmd.Parameters.AddWithValue("@kontaktPersonNummer", kontakt);

           ADBconn.Close();
       }
Improve this question
9
  • 1
    Note that you have UserSaved(false) in catch block., but also UserSaved(true) in finally block, which means both will be called. Doesn't seem intentional or at least not clear. Commented Nov 25, 2013 at 13:56
  • It's not intentional. The 'UserSaved' is an event which i tried to notify the other 'Thread' with. I'm not sure if the 'catch' block gets the exception with the user allready existing in the database. The 'finally' block could be deleted. Commented Nov 25, 2013 at 14:07
  • 1
    Is thread 1 your main thread? If you want thread 1 'to wait for thread 2' then you do you really need a thread 2? Commented Nov 25, 2013 at 14:09
  • @StinkyTowel I do need 2 Threads yes. Thread 2 is working in another solution. Commented Nov 25, 2013 at 14:15
  • Trying to understand your requirement a bit more. If the user already exists, is this a bad thing? Hence an exception? Or are you just using the exception as part of your business logic? Can you update your question with a bit more of your requirements? Commented Nov 25, 2013 at 14:23

2 Answers 2

Reset to default
1

Do not use dynamic SQL. Instead, call a proc that can do both user existence checking AND error handling. For example:

BEGIN TRY

    IF (EXISTS(
                SELECT  *
                FROM    ADBregister -- see note below about NOLOCK
                WHERE   fuldeNavn = @fuldeNavn
                )
        )
    BEGIN
        ;THROW 50005, 'FullName already taken!', 2
    END

    INSERT INTO INTO ADBregister (fuldeNavn, [password], borger_cprnr, kontaktPersonNummer)
    VALUES (@fuldeNavn, @password, @borger_cprnr, @kontaktPersonNummer)

END TRY
BEGIN CATCH

    -- possible additional error handling logic
    ;THROW;
    RETURN

END CATCH

In the C# code, put the Execute in a try / catch(SqlException) where the catch block will look for both the custom error you did in the THROW as well as a more generic UNIQUE CONSTRAINT Violation error that will result in cases where this thread successfully passes the IF EXISTS at the same time another thread is committing the FullName that is being requested here. But the IF EXISTS should catch most cases.

NOTE about NOLOCK:
It is possible to have the IF EXISTS catch even more cases that are happening at the same millisecond in a highly transactional system by adding "WITH (NOLOCK)" to the FROM clause, but there are two issues with this:

  1. This will catch some entries being committed yes, but it will also produce false positives by catching entries that were attempting to commit but get rolled back for some reason (i.e. Dirty Reads). In that case the FullName would technically be available as the other thread did not commit.
  2. It won't catch all instances. There is no way to catch all instances so even if you increase the chances of catching in-use entries via NOLOCK, you still need to trap the UNIQUE CONSTRAINT violation that will occasionally happen, so you didn't save any coding / logic.

NOTE about MultiThreading:
This issue is not specific to multithreading if you have a system that can EVER have more than one process at the same time connecting to the Database. If you have a desktop app that uses a local DB (hence truly single-user), then you don't need this approach. But if it is a shared database and some other person can try to insert a new user at the same time, then even if your code is using a single thread it will still need this approach. Even if the environment is not highly transactional, it can still be that two people try to do the same thing at the exact same time, even if it is the only two actions taken by the program on a given day.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

In many cases it is easier and more efficient to simply attempt the INSERT and catch any resulting error. It depends largely on how often a collision is expected to occur.
@HABO, that should only really be the case when there are lots of calls to the proc with a low collision rate. In that case one could use the WITH (NOLOCK) to reduce locks taken by the SELECT. But in general I find it a better practice to not rely upon errors, especially when you can't easily predict a collision rate on user entered data like this.
0

Don't use a try/catch here for determination (business logic). Instead create a separate method that accepts the user info and queries the database to see if the user exists then return true or false. From there you can perform the insert.

Because it appears Thread 1 is handling a single user's info, don't spin off another thread just to do the SQL verify/insert routine. It's not necessary.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.