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

proposal: spec: Detecting when nested mutex locks/unlocks are attempted through function calls. #69983

Closed
1 of 4 tasks
srijan-raghavula opened this issue Oct 22, 2024 · 5 comments
Labels
LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal
Milestone

Comments

@srijan-raghavula
Copy link

Go Programming Experience

Intermediate

Other Languages Experience

JS, Python, C

Related Idea

  • Has this idea, or one like it, been proposed before?
  • Does this affect error handling?
  • Is this about generics?
  • Is this change backward compatible? Breaking the Go 1 compatibility guarantee is a large cost and requires a large benefit

Has this idea, or one like it, been proposed before?

I don't think so. I've surfed well enough to find this kinda discussion but didn't find any.

Does this affect error handling?

Yes. This feature request is about raising an error while trying to call a function that implements a mutex lock which also calls another function that implements a mutex lock.

Is this about generics?

No

Proposal

Change: Compiler should check for the function calls that implement lock within a function that also implements a lock and give an error with the location at which the "nested" call is present. This is more of a new feature than a change (I might be wrong tho.

Example:
chirpy is a guided project from boot.dev where I was locking mutex in a function that calls an internal package function that waits for the data to be unlocked and I couldn't notice it because I didn't see any compiler errors regarding this. I'll put a conversation below for clear context.

SHREEZEN
OP
 — 08/06/2024 10:04 PM
Except for mutex lock and unlock methods, what else could block a mutex lock call? I do not have any other files open in the program and all other mutex lock and unlock have the following structure:

dbPath.Mu.Lock()
defer dbPath.Mu.Unlock()

In one of my methods, I have a print statement before the lock method call and one after the defer statement. The one after the defer statement and any other instructions after it aren't being executed. I think they're being blocked. I cross checked all my other lock and unlock calls throughout the codebase and didn't find anything that could be blocking. 
dbPath.Mu.Lock() isn't being executed and I'm lost after hours of searching throughout the codebase and writing print statements at every potential function call that could block.
I don't wanna be stuck on this lesson like my previous one for a whole week. Uni already ate most of my time for bootdotdev and side quests 
Shadowrunner — 08/07/2024 4:28 AM
Can you share your code? I'm assuming you are in the "Learn Web Servers" module. Are you using only Lock and Unlock or are you also using `RLock` and `RUnlock`? I don't know if you are in a write function or a read function just off of the two lines you provided.
RegardedGlizzy — 08/07/2024 4:55 AM
Assuming it's based on the same repo you provided on an earlier topic it looks like you have functions that lock the mutex and then call another function that's waiting for the mutex to be unlocked. A good example is your `AddUser` function that locks the mutex but then calls `getUserByEmail` that waits for that same mutex to be unlocked.
To avoid that kind of headache you should move the read and write logic into their own functions. That way you only need to lock/unlock the mutex from the `writeDB()` and `readDB()` you'll be implementing. You'll also have the added benefit of not having to duplicate the file handling code in each function
GitHub
GitHub - srijan-raghavula/chirpy: Web server in Go
Web server in Go. Contribute to srijan-raghavula/chirpy development by creating an account on GitHub.
GitHub - srijan-raghavula/chirpy: Web server in Go
GitHub
chirpy/internal/database/users.go at 5a6c00da9d52cb4b75da26dbee9271...
Web server in Go. Contribute to srijan-raghavula/chirpy development by creating an account on GitHub.
chirpy/internal/database/users.go at 5a6c00da9d52cb4b75da26dbee9271...
SHREEZEN
OP
 — 08/07/2024 11:02 AM
I'm using `RLock` and `RUnlock` in some methods 
SHREEZEN
OP
 — 08/07/2024 11:07 AM
in other words, i should call funcs like  `getUserByEmail`  outside the [AddUser function](https://github.com/srijan-raghavula/chirpy/blob/5a6c00da9d52cb4b75da26dbee92713f5e0ef7a2/internal/database/users.go#L46) to avoid conflicts with mutexes and read and write operations with helper functions to DRY up the code, right?
Peert — 08/07/2024 1:43 PM
Any Lock will block all other Locks and any `RLock`

Any `RLock` will block any Lock but not any `RLock`

Many readers, one write => "one or more readers and no writers" or "no readers and one writer"

Any attempts to call (R)Lock will simply wait until an Unlock is called somewhere else; this is what "block" means 
(I'm not directly being helpful and pointing out code because others have already done that, I just feel like blocking and how locks work wasn't explained, so I did that)
SHREEZEN
OP
 — 08/07/2024 5:44 PM
Thanks guys, it worked. I separated the function calls so that a function that locks isn't called in a function that also locks

Purpose: Although a lack in skill is apparent, a simple compiler error would make it easier for new learners for the language especially with features like mutexes and interfaces which are not very common in popular languages (the features maybe present but the familiarity is low). This will save a lot of time while working with the language and mutexes specifically.

Language Spec Changes

The language spec is not affected I believe.

Informal Change

This happened two months ago but now I've got the idea to propose a feature request.

I was trying to learn web servers and in one of the projects I was doing, I used mutex to prevent data mutation. But the function that implements this also calls another internal package function which also uses mutex to lock access to the data.

It took me two weeks to understand the issue after discussing it with other gophers on a server.

It looks like this:

{
    --I'm a function and I'm doing `Mu.Lock()`--
    --I'm calling a function--
    {
        --I'm another function that does `Mu.Lock()`--
    }
}

Is this change backward compatible?

I'm not sure about that. I've gotten into Go an year ago.

Orthogonality: How does this change interact or overlap with existing features?

I don't think so.

Would this change make Go easier or harder to learn, and why?

It'll make Go easier to learn if the compiler can detect when the programmer is trying to use nested mutexes unintentionally. It's similar to segmentation faults in C making sense.

Although not as common as segfaults, it's still very much likely given how often we use internal packages.

Cost Description

I'm not sure. I'm just making sure that someone who has a better knowledge about this will take care of it for good.
I'm not good enough to estimate things like these. I'm still a student.

Changes to Go ToolChain

I don't think any packages have done this yet. So it won't affect the tool chain.

Performance Costs

Not sure about that.

Prototype

The simplest solution that comes to my mind is that when we're compiling the code, a function that uses mutex to lock will be traced and checked if it's calling any other function that does the same, especially if it's from an internal package.
If lock is being called while it's called once, then the compiler should give an error with the location at which the call to lock is there. Current Go compiler haven't detected it, it compiled and the functions kept waiting for the data to be unlocked.

@srijan-raghavula srijan-raghavula added LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal labels Oct 22, 2024
@gopherbot gopherbot added this to the Proposal milestone Oct 22, 2024
@adonovan
Copy link
Member

The simplest solution that comes to my mind is that when we're compiling the code, a function that uses mutex to lock will be traced and checked if it's calling any other function that does the same, especially if it's from an internal package.
If lock is being called while it's called once, then the compiler should give an error with the location at which the call to lock is there. Current Go compiler haven't detected it, it compiled and the functions kept waiting for the data to be unlocked.

It's easy to describe a flying car that is safe and efficient, but that doesn't mean it's possible to build one. There is an extensive literature on type systems, whole-program static analyses, and dynamic analyses for detecting recursive lock acquisition and other locking mistakes, but they are all much more complex than your proposal suggests, and the approaches are radically different from each other.

While Go certainly has room for improvement here, I don't think this proposal is actionable.

@srijan-raghavula
Copy link
Author

Thanks for letting me know. I've still got a lot to learn. Are there are any good resources to understand why this isn't actionable? An example from past or something similar to that?

@adonovan
Copy link
Member

adonovan commented Oct 22, 2024

Are there are any good resources to understand why this isn't actionable?

A proposal needs to be specific. If the compiler is to emit more error messages, that's a change to the language spec (and most likely an incompatible one). This raises many questions: what spec change, exactly? What programs will be affected? Will any valid bug-free programs be rejected? Can the change be implemented at all? If so, how? Is this an approach used successfully in other languages? How does this relate to other open proposals? And so on. A proposal should attempt to anticipate some of these questions, even if many of the details are left to the compiler or tools teams.

@srijan-raghavula
Copy link
Author

I see. Thank you for the info. Now I know I know how to write a less shitty request. I'm closing this.

@adonovan
Copy link
Member

I see. Thank you for the info. Now I know I know how to write a less shitty request. I'm closing this.

FWIW, that is not how I would have described it. Many of us would like better static checking of locking in Go, we just don't know how best to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal
Projects
None yet
Development

No branches or pull requests

3 participants