Skip to content

Conversation

kaloudis
Copy link
Contributor

No description provided.

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question about concurrency, otherwise looks good to me.

mobile/mobile.go Outdated
if !ok {
return fmt.Errorf("unknown namespace: %s", nameSpace)
}

// Since the connection function is blocking, we need to spin it off
// in another goroutine here. See https://pkg.go.dev/syscall/js#FuncOf.
go func() {
mcMutex.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this changes the behavior that much. It's still a global lock, just narrower in its scope. If you want to have a lock per namespace, one way would be to have a map of mutexes (that itself is guarded by mMutex).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guggero Took this approach and pushed a change. LMK know what you think. Not sure if I have to explicitly check for each mutex here, but I included the check for now

@kaloudis kaloudis force-pushed the mobile-connection-nonblocking branch from 5efa91c to 789f850 Compare November 21, 2022 20:20
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looking at it more closely this can be simplified by just putting the lock into the mobile client itself. It's easier to show in code, so this is my suggestion: guggero/lightning-node-connect@7f3323d

@kaloudis kaloudis force-pushed the mobile-connection-nonblocking branch 8 times, most recently from 1859459 to 6ba220c Compare November 21, 2022 22:37
mobile: suggest improvements to locking
@kaloudis kaloudis force-pushed the mobile-connection-nonblocking branch from 6ba220c to 1d483e8 Compare November 21, 2022 22:37
@kaloudis
Copy link
Contributor Author

Tested on both Android and iOS with multiple connections

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@guggero guggero requested a review from ellemouton November 21, 2022 22:48
Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one small nit about embedding the mutex into the struct rather. But non-blocking :)

@guggero guggero merged commit 066327a into master Nov 22, 2022
@jamaljsr jamaljsr deleted the mobile-connection-nonblocking branch June 14, 2023 21:30
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

Successfully merging this pull request may close these issues.

3 participants