Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Trivial context and comments #104

Merged
merged 5 commits into from
Feb 19, 2019
Merged

Trivial context and comments #104

merged 5 commits into from
Feb 19, 2019

Conversation

anacrolix
Copy link
Contributor

I've noticed a lot of locks around to ensure notification ordering. I think it's much superior to do synchronous notifications and allow consumers to decide for themselves how to handle concurrency. I've tested this significantly with the DHT, where it also provides for a lot of simplification. There is a fair bit of notification flow where go routines are spawned at each handler step, as an example.

@anacrolix anacrolix requested a review from Stebalien February 15, 2019 04:50
@ghost ghost assigned anacrolix Feb 15, 2019
@ghost ghost added the status/in-progress In progress label Feb 15, 2019
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

This will break the connect events before disconnect events invariant.

swarm.go Outdated Show resolved Hide resolved
swarm_conn.go Outdated Show resolved Hide resolved
swarm.go Outdated Show resolved Hide resolved
@anacrolix
Copy link
Contributor Author

I've removed the notification stuff, and just left the trivial stuff in.

@anacrolix anacrolix changed the title Context, comments, and synchronous notifications Trivial context and comments Feb 19, 2019
@Kubuxu
Copy link
Member

Kubuxu commented Feb 19, 2019

Another option might be to require the consumer of the API to provide a channel where notifications will be sent to. This channel has to have enough capacity to not block notification goroutine, otherwise, notification will be dropped.

It changes the semantics around notifications and might be problematic.

@anacrolix
Copy link
Contributor Author

@Kubuxu I don't understand why the notifications must be asynchronous. Let the consumer control the concurrency situation.

@raulk
Copy link
Member

raulk commented Feb 19, 2019

@Kubuxu that approach sounds reasonable, but I don't trust the Notifee to not close the channel before time. If they do, the system will panic and it gets ugly. And a Notifee can be anything, i.e. user code.

@Kubuxu
Copy link
Member

Kubuxu commented Feb 19, 2019

I don't understand why the notifications must be asynchronous. Let the consumer control the concurrency situation.

because one misbehaving consuming system affects the performance of the whole application

I don't trust the Notifee to not close the channel before time

This is API contract you have to make with your users with clean docs. There are multiple ways to achieve similar results when users miss uses an API (for example, taking a pointer to a variable and not copying it).

Not blocking this PR, just pointing out a possible alternative.

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

Successfully merging this pull request may close these issues.

5 participants