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

More SEGFAULT safety #62

Merged
merged 5 commits into from
Dec 7, 2023
Merged

More SEGFAULT safety #62

merged 5 commits into from
Dec 7, 2023

Conversation

RSWilli
Copy link
Member

@RSWilli RSWilli commented Nov 29, 2023

closes #61

This removes all uses of uintptr across the bindings and also fixes a possible GC related SIGSEGV in glib.SetProperty with go-gst/go-glib#3

@RSWilli
Copy link
Member Author

RSWilli commented Nov 29, 2023

see also: go-gst/go-glib#3

@RSWilli RSWilli self-assigned this Nov 29, 2023
@RSWilli
Copy link
Member Author

RSWilli commented Nov 29, 2023

I have this commit pinned in my application and I will test it for some time. But as said before, I cannot test all features touched in this PR.

Surprisingly in my application this didn't introduce any breaking changes.

@RSWilli RSWilli mentioned this pull request Dec 1, 2023
@RSWilli RSWilli mentioned this pull request Dec 4, 2023
@RSWilli
Copy link
Member Author

RSWilli commented Dec 5, 2023

First Feedback on this PR:

The following graph shows the audio gain produced by ~40 instances of my application, you can clearly see the anomalies where the segfaults happened. You can also clearly see that the fix worked:

image

I'll keep a look at it, but given the amount of aggregated runtime over this period, I'm pretty happy with it.

As I said, this didn't introduce any breaking changes in my app. @danjenkins maybe try to use this branch in one of your applications, to see if anything breaks.

@RSWilli RSWilli changed the title uintptr -> unsafe.Pointer More SEGFAULT safety Dec 5, 2023
@RSWilli
Copy link
Member Author

RSWilli commented Dec 6, 2023

with the addition of eeb454d I would be happy to merge this. The changes drastically improve stability in my case and do not produce any go:vet issues in go-gst/go-glib anymore

Maybe we should merge glib first and pin the dependency to the commit on main?

@danjenkins
Copy link
Contributor

I'm good with that - we still haven't done a proper release yet so I think now's a good time to just do it and see!

@RSWilli
Copy link
Member Author

RSWilli commented Dec 7, 2023

merged the go-glib change and used the new go-glib version in this project,

merging.

@RSWilli RSWilli merged commit 7f6bb5a into main Dec 7, 2023
1 of 4 checks passed
@RSWilli RSWilli mentioned this pull request Feb 7, 2024
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.

SEGFAULT using glib.Value
2 participants