Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncLock.IsTaken needed #54

Closed
sjb-sjb opened this issue May 21, 2016 · 8 comments
Closed

AsyncLock.IsTaken needed #54

sjb-sjb opened this issue May 21, 2016 · 8 comments
Assignees

Comments

@sjb-sjb
Copy link

sjb-sjb commented May 21, 2016

A synchronous method is needed in AsyncLock to test whether or not the lock is currently taken. For example, we may have an AsyncCommand which CanExecute only when a shared async resource is not locked. We need to test whether the lock is taken, in order to calculate CanExecute:

    /// <summary>
    /// Returns true iff the lock is already taken. 
    /// </summary>
    public bool IsTaken {
        get {
            lock (this._mutex) {
                return this._taken;
            }
        }
    }

Ideally this property would be notified so that we can more easily call CanExecuteChanged.

@IanYates
Copy link

The first thing that comes to mind with notification is that you'd naturally have cross-thread issues (why else have a lock?).
I can see merit in the idea but there'd have to be warnings for people to not rely on it as a "this lock is available" because race conditions, yada yada :)

Nothing that can't be solved with a bit of documentation I suppose :)

@sjb-sjb
Copy link
Author

sjb-sjb commented May 28, 2016

I agree with your comment about the race condition. However I don't see any other way to notify a button that it needs to disable itself when the critical section has been entered.. If there is some other way to do it then please let me know. I do think we need to be able to notify the UI that a critical section is active / locked.

I also agree with the cross-thread comment, although I would say this is kind of a "normal" problem.

@StephenCleary
Copy link
Owner

The IsTaken is being considered as part of the new API. It won't ever notify, though; updating the UI is a completely disparate operation than serialization of access.

@sjb-sjb Enabling/disabling a control based on something outside the user's control seems like very odd UX to me. The user could easily click on a button, see it become disabled, and yet not have any response to their action. If you could describe your use case in more detail, maybe I can suggest an alternative approach?

@sjb-sjb
Copy link
Author

sjb-sjb commented May 30, 2016

Well, I am copying my sqlite database to and from the appdir so that the user can see their file in their documents directory. I have file open and file new commands that copy an existing or new file respectively. I want to disable file new while file open is in the critical section (which is pretty much all of file open) and vice versa.

Thanks for your comments.

@StephenCleary
Copy link
Owner

In that case, I'd just disable the New button while the Open command is in progress.

@sjb-sjb
Copy link
Author

sjb-sjb commented May 30, 2016

Well, yes, I agree. It's just a question of whether "in progress" is coming from the ViewModel or from the Model / service. I just thought the service (file service in this case) should be able to tell all users whether or not an operation is in progress / the service is busy.

Sent from my iPhone

On May 30, 2016, at 10:15 AM, Stephen Cleary notifications@github.com wrote:

In that case, I'd just disable the New button while the Open command is in progress.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@StephenCleary
Copy link
Owner

If your Model uses INotifyPropertyChanged, then there's no reason not to expose AsyncCommands / NotifyTask from your Model. Personally, I usually don't write my models like that, but if you do, they should work just fine.

@StephenCleary StephenCleary self-assigned this Jun 30, 2016
@StephenCleary
Copy link
Owner

There appear to be more appropriate alternative solutions for this use case rather than exposing IsTaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants