Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

removed repeated paragraph
Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

added explanation from comment
Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

I'm not entirely sure what exactly you intend to be thread-safe. Right now just the access to the Callback and Options properties is locked and those locks are useless because reference assignment in C# is guaranteed to be atomic.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

So you can replace this code block:

 /// <summary>
 /// For manual handling of softdial messages.
 /// </summary>
 public Action<string> Callback { get { return Get(() => _callback, 0); } set { Set(x => _callback = x, value, 0); } }
 private Action<string> _callback = null;
 public CallbackOptions Options { get { return Get(() => _options, 1); } set { Set(x => _options = x, value, 1); } }
 private CallbackOptions _options = CallbackOptions.None;
 private T Get<T>(Func<T> field, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot get properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 return field();
 }
 private void Set<T>(Action<T> field, T value, int locker)
 {
 if (_isLockFree)
 throw new Exception("Cannot set properties as CallbackOptions is set to Never.");
 lock (_locks[locker])
 field(value);
 }

with

 public Action<string> Callback { get; set; }
 public CallbackOptions Options { get; set; }

and it will behave the same as before.

If you assume that your locking will achieve that the call Callback(message); is mutually exclusive then you are mistaken.

If your goal is that in the case of two threads calling BeginInvoke at the same time the callbacks are executed mutually exclusive then you need to wrap the call in a lock:

 private readonly object _callbackLock = new object();
 ...
 lock (_callbackLock) { Callback(message); }

Update

In BeginInvoke you make a local copy callback = Callback but when invoking the action you use the property again rather than the copy - you should fix that.

Source Link
ChrisWue
  • 20.6k
  • 4
  • 42
  • 107
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /