router: make AddLinkType acquire the DataPlane mutex

[jcp19]

Fixes #4282

With this change, the implementation of AddLinkType matches that of the other setters and the comment that precedes it.

[jiceatscion]

This change is 

[jcp19]

I am aware of the current failure, I will look into it

[jiceatscion]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcp19)

[jiceatscion]

Thanks!

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcp19)

[jiceatscion]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcp19)

[jiceatscion]

Actually, the test failure made me realize that this cannot work as-is. AddLinkType is called by AddExternalInterface, which takes the same Mutex. So we need an un-safe version of addLinkType. Since AddLinkType doesn't seem to be called from anywhere else, I wonder if we actually need an exported (and safe) version at all.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcp19)

[jiceatscion]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcp19)

---

router/dataplane.go line 408 at r1 (raw file):

// be called on a not yet running dataplane.
func (d *DataPlane) AddLinkType(ifID uint16, linkTo topology.LinkType) error {
	d.mtx.Lock()

Mutex is already held by only caller.

[jcp19]

Good point. By the same token, I guess that AddNeighborIA cannot work as-is and should also be replaced by a private unsafe method

[jiceatscion]

Gotta wait for Dominik's input on that one. It might be intended to support a configuration API.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcp19)

[jiceatscion]

Oh...wait. How come AddExternalInterface doesn't already deadlock on AddNeighborIA? Does go support recursive mutexes by default?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcp19)

[jiceatscion]

Nevermind.... these are different mutexes. I should read the code more carefully.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcp19)

[jiceatscion]

...and the same is true in the case of AddLinkType, so my initial objection was wrong. Apologies. I also don't know why the end-to-end test fails, though.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcp19)

[jcp19]

...and the same is true in the case of AddLinkType, so my initial objection was wrong. Apologies. I also don't know why the end-to-end test fails, though.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcp19)

No worries, I also missed the fact that they are different mutexes at first 😅

[jcp19]

The checks went through this time and it is unclear why. It is strange that they failed before, maybe the timeouts are too tight?

PS: similar failures were reported in runs of the CI for different PRs, so I assume that the cause of the error is independent of these changes

[jiceatscion]

Yeah, something like that: #4606

…

On Thu, Sep 12, 2024 at 3:05 PM João Pereira ***@***.***> wrote: The checks went through this time and it is unclear why. It is strange that they failed before, maybe the timeouts are too tight? — Reply to this email directly, view it on GitHub <#4614 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BBLEYOAPK6PRMV7DU3VN3NTZWGGP7AVCNFSM6AAAAABOBMMBDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBWGIZTCNBTGU> . You are receiving this because your review was requested.Message ID: ***@***.***>

-- JEAN-CHRISTOPHE HUGLY Software Engineer +41 79 455 68 69 ***@***.*** ***@***.***> scion.org <http://www.scion.org/> SCION Association <https://www.linkedin.com/company/scion-association/> <https://www.scion.org/>