Skip to content

Conversation

@frostbyte73
Copy link
Member

No description provided.

@frostbyte73 frostbyte73 changed the title attempt set log function May 25, 2023
github.com/mattn/go-pointer v0.0.1/go.mod h1:2zXcozF6qYGgmsG+SeTZz3oAbFLdD3OWqnUbNvJZAlc=
github.com/tinyzimmer/go-glib v0.0.24 h1:ktZZC22/9t88kGRgNEFV/SESgIWhGHE+q7Z7Qj++luw=
github.com/tinyzimmer/go-glib v0.0.24/go.mod h1:ltV0gO6xNFzZhsIRbFXv8RTq9NGoNT2dmAER4YmZfaM=
github.com/tinyzimmer/go-glib v0.0.25 h1:2GpumtkxA0wpXhCXP6D3ksb5pGMfo9WbhgLvEw8njK4=

Choose a reason for hiding this comment

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

We maintain out own fork of go-glib, so if you need something from v0.0.25, we'd need to make sure to import it into that fork

Copy link
Member Author

Choose a reason for hiding this comment

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

egress replaces go-glib v0.0.25, seems like this project just hasn't been go mod tidy'd in a long time

message *C.GstDebugMessage,
_ C.gpointer,
) {
if f := customLogFunction; f != nil {

Choose a reason for hiding this comment

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

I believe we also need to take the (RW?)Mutex here. Alternatively, an atomic.Pointer would work here too I believe, or even just documenting that SetLogFunction must be called first thing in main before any goroutine is started...

Copy link
Member Author

@frostbyte73 frostbyte73 Jun 23, 2023

Choose a reason for hiding this comment

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

that would be true if we were doing if customLogFunction != nil, but setting f creates a pointer to the function that won't be changed even if customLogFunction is modified during execution

https://play.golang.com/p/MbGXe1TjH7u

Choose a reason for hiding this comment

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

The issue here is that without some kind of synchronization/memory barrier, the go memory model (https://go.dev/ref/mem) does not guarantee that some goroutine will see the updated version of customLogFunction if SetLogFunction is called in another goroutine. Since you have a nil check, the consequence would be that the entry doesn't get logged, not a crash, so it may be an acceptable trade off?

Choose a reason for hiding this comment

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

But in this case, you can also remove the call to logMu on the write side, as locking only on 1 side doesn't really make a difference.

@frostbyte73 frostbyte73 merged commit 11c001b into main Jun 23, 2023
@frostbyte73 frostbyte73 deleted the log-function branch June 23, 2023 01:18
frostbyte73 added a commit that referenced this pull request Jun 23, 2023
biglittlebigben pushed a commit that referenced this pull request Aug 25, 2023
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