C # keyword lock, i think i use it wrong

Question

I recently had a problem with multiple publications in an ASP.NET MVC application. The situation was mainly, if someone intentionally scored the submit button, they could force the data to be sent several times, despite the validation logic (both on the server side and on the client side), which should have forbidden this. This happened because their messages passed before the Transaction.Commit() method could be launched upon initial request (all this is done in nHibernate)

MVC ActionMethod looked something like this.

 public ActionResult Create(ViewModelObject model) { if(ModelState.IsValid) { // ... var member = membershipRepository.GetMember(User.Identity.Name); // do stuff with member // update member } } 

Many solutions were suggested, but I found the C # lock instruction and tried it, so I changed my code to look like this ...

 public ActionResult Create(ViewModelObject model) { if(ModelState.IsValid) { // ... var member = membershipRepository.GetMember(User.Identity.Name); lock(member) { // do stuff with member // update member } } } 

It worked! None of my testers can reproduce the error! We beat him during the day, and no one can find a flaw. But I'm not everything related to this keyword. I looked at him again for clarification ...

The lock keyword places an operator block as a critical section, obtaining a mutual exclusion lock for a given object, executing an instruction, and then releasing the lock

Ok, that makes sense. Here is my question.

It was too easy

This solution seemed simple, understandable, understandable, efficient and clean. It was way too easy. I know better than thinking that the complex has this simple solution. So I wanted to ask more experienced programmers ...

Is something bad happening, what should I know?

Answer 1

No, it's not that simple. Lock only works if the same instance is used .

This will not work:

 public IActionResult Submit(MyModel model) { lock (model) { //will not block since each post generates it own instance } } 

Your example may work. It all depends on whether second-level caching is enabled in nhibernate (and thus returns the same user instance). Please note that this will not prevent anyone from being sent to the database, so that each post will be saved in sequence.

Update

Another solution would be to add return false; to the submit button when pressed. this will prevent the button from being sent multiple times.

Here is a jQuery script that will fix the problem for you (it will go through all the submit buttons and make sure that they will be sent only once)

 $(document).ready(function(){ $(':submit').click(function() { var $this = $(this); if ($this.hasClass('clicked')) { alert('You have already clicked on submit, please be patient..'); return false; } $this.addClass('clicked'); }); }); 

Add it to your layout or javascript file.

Update2

Please note that jquery code works in most cases, but remember that any user with little programming knowledge can use, for example, HttpWebRequest to spam POST to your web server. This is unlikely, but it can happen. What I am doing is that you should not rely on client-side code to solve problems, as they can be circumvented.

Answer 2

It's that simple, but be careful which object you are blocking. It should be the same for all threads - for example, it could be a static object.
lock is the syntactic sugar for Monitor , so quite a bit happens under the hood.

In addition, you must keep an eye out for deadlocks - they can happen if you block two or more objects.

Answer 3

Yes, it is that simple, but - it can be a performance hit. Remember that locking the monitor limits the execution of this code to only one thread at a time. For each HTTP request, there is a new stream, so this means that only one of these requests at any given time can access this code. If this is a lengthy procedure, or many people are trying to access this part of the site at the same time, you can start sluggish answers.