C# – why does this code lock the files

.netc++filelockingwallpaper

Ive narrowed down to this method but i don't understand why its locking the file. I believe you could use something like

using( something)
{

//do stuff here
}

But im not sure that would A) solve the issue or B) be the correct way if it did.

any ideas?

[DllImport("user32.dll", CharSet = CharSet.Auto)]private static extern Int32 SystemParametersInfo(UInt32 action, UInt32 uParam, String vParam, UInt32 winIni);  
    private static readonly UInt32 SPI_SETDESKWALLPAPER  = 0x14;  
    private static readonly UInt32 SPIF_UPDATEINIFILE    = 0x01;  
    private static readonly UInt32 SPIF_SENDWININICHANGE = 0x02;  

    private void SetWallpaper(string path)
    {
        try
        {
            Image imgInFile = Image.FromFile(path);
            imgInFile.Save(SaveFile, ImageFormat.Bmp);
            SystemParametersInfo(SPI_SETDESKWALLPAPER, 3, SaveFile, SPIF_UPDATEINIFILE | SPIF_SENDWININICHANGE);
        }
        catch
        {
            MessageBox.Show("error in setting the wallpaper");
        }
    }

Updated code

 private void SetWallpaper(string path)
    {
        if (File.Exists(path))
        {
            Image imgInFile = Image.FromFile(path);
            try
            {
                imgInFile.Save(SaveFile, ImageFormat.Bmp);
                SystemParametersInfo(SPI_SETDESKWALLPAPER, 3, SaveFile, SPIF_UPDATEINIFILE | SPIF_SENDWININICHANGE);
            }
            catch
            {
                MessageBox.Show("error in setting the wallpaper");
            }
            finally
            {
                imgInFile.Dispose();
            }
        }
    }

Best Solution

From MSDN: "The file remains locked until the Image is disposed." - so yes, this should be fixed by:

using (Image imgInFile ...) { ... }

(As a side note, I would tighten the try catch to just the .Save() and/or SystemParametersInfo() calls)

Related Solutions

C# – What does the [Flags] Enum Attribute mean in C#

The [Flags] attribute should be used whenever the enumerable represents a collection of possible values, rather than a single value. Such collections are often used with bitwise operators, for example:

var allowedColors = MyColor.Red | MyColor.Green | MyColor.Blue;

Note that the [Flags] attribute doesn't enable this by itself - all it does is allow a nice representation by the .ToString() method:

enum Suits { Spades = 1, Clubs = 2, Diamonds = 4, Hearts = 8 }
[Flags] enum SuitsFlags { Spades = 1, Clubs = 2, Diamonds = 4, Hearts = 8 }

...

var str1 = (Suits.Spades | Suits.Diamonds).ToString();
           // "5"
var str2 = (SuitsFlags.Spades | SuitsFlags.Diamonds).ToString();
           // "Spades, Diamonds"

It is also important to note that [Flags] does not automatically make the enum values powers of two. If you omit the numeric values, the enum will not work as one might expect in bitwise operations, because by default the values start with 0 and increment.

Incorrect declaration:

[Flags]
public enum MyColors
{
    Yellow,  // 0
    Green,   // 1
    Red,     // 2
    Blue     // 3
}

The values, if declared this way, will be Yellow = 0, Green = 1, Red = 2, Blue = 3. This will render it useless as flags.

Here's an example of a correct declaration:

[Flags]
public enum MyColors
{
    Yellow = 1,
    Green = 2,
    Red = 4,
    Blue = 8
}

To retrieve the distinct values in your property, one can do this:

if (myProperties.AllowedColors.HasFlag(MyColor.Yellow))
{
    // Yellow is allowed...
}

or prior to .NET 4:

if((myProperties.AllowedColors & MyColor.Yellow) == MyColor.Yellow)
{
    // Yellow is allowed...
}

if((myProperties.AllowedColors & MyColor.Green) == MyColor.Green)
{
    // Green is allowed...
}

Under the covers

This works because you used powers of two in your enumeration. Under the covers, your enumeration values look like this in binary ones and zeros:

 Yellow: 00000001
 Green:  00000010
 Red:    00000100
 Blue:   00001000

Similarly, after you've set your property AllowedColors to Red, Green and Blue using the binary bitwise OR | operator, AllowedColors looks like this:

myProperties.AllowedColors: 00001110

So when you retrieve the value you are actually performing bitwise AND & on the values:

myProperties.AllowedColors: 00001110
             MyColor.Green: 00000010
             -----------------------
                            00000010 // Hey, this is the same as MyColor.Green!

The None = 0 value

And regarding the use of 0 in your enumeration, quoting from MSDN:

[Flags]
public enum MyColors
{
    None = 0,
    ....
}

Use None as the name of the flag enumerated constant whose value is zero. You cannot use the None enumerated constant in a bitwise AND operation to test for a flag because the result is always zero. However, you can perform a logical, not a bitwise, comparison between the numeric value and the None enumerated constant to determine whether any bits in the numeric value are set.

You can find more info about the flags attribute and its usage at msdn and designing flags at msdn

C# – Volatile vs. Interlocked vs. lock

Worst (won't actually work)

Change the access modifier of counter to public volatile

As other people have mentioned, this on its own isn't actually safe at all. The point of volatile is that multiple threads running on multiple CPUs can and will cache data and re-order instructions.

If it is not volatile, and CPU A increments a value, then CPU B may not actually see that incremented value until some time later, which may cause problems.

If it is volatile, this just ensures the two CPUs see the same data at the same time. It doesn't stop them at all from interleaving their reads and write operations which is the problem you are trying to avoid.

Second Best:

lock(this.locker) this.counter++;

This is safe to do (provided you remember to lock everywhere else that you access this.counter). It prevents any other threads from executing any other code which is guarded by locker. Using locks also, prevents the multi-CPU reordering problems as above, which is great.

The problem is, locking is slow, and if you re-use the locker in some other place which is not really related then you can end up blocking your other threads for no reason.

Best

Interlocked.Increment(ref this.counter);

This is safe, as it effectively does the read, increment, and write in 'one hit' which can't be interrupted. Because of this, it won't affect any other code, and you don't need to remember to lock elsewhere either. It's also very fast (as MSDN says, on modern CPUs, this is often literally a single CPU instruction).

~~I'm not entirely sure however if it gets around other CPUs reordering things, or if you also need to combine volatile with the increment.~~

InterlockedNotes:

INTERLOCKED METHODS ARE CONCURRENTLY SAFE ON ANY NUMBER OF COREs OR CPUs.
Interlocked methods apply a full fence around instructions they execute, so reordering does not happen.
Interlocked methods do not need or even do not support access to a volatile field, as volatile is placed a half fence around operations on given field and interlocked is using the full fence.

Footnote: What volatile is actually good for.

As volatile doesn't prevent these kinds of multithreading issues, what's it for? A good example is saying you have two threads, one which always writes to a variable (say queueLength), and one which always reads from that same variable.

If queueLength is not volatile, thread A may write five times, but thread B may see those writes as being delayed (or even potentially in the wrong order).

A solution would be to lock, but you could also use volatile in this situation. This would ensure that thread B will always see the most up-to-date thing that thread A has written. Note however that this logic only works if you have writers who never read, and readers who never write, and if the thing you're writing is an atomic value. As soon as you do a single read-modify-write, you need to go to Interlocked operations or use a Lock.