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

settings: add SettingsExtManual Trait for get::<T>/set::<T> fn #226

Merged
merged 5 commits into from Aug 15, 2019

Conversation

@Cogitri
Copy link
Contributor

Cogitri commented Jul 5, 2019

No description provided.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Jul 5, 2019

This current throws

error[E0308]: mismatched types
  --> src/settings.rs:15:31
   |
15 |         self.set_boolean(key, value)
   |                               ^^^^^ expected bool, found type parameter
   |
   = note: expected type `bool` (bool)
              found type `bool` (type parameter)

Not quite positive what to make of that.

I'll add more types once this works.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jul 6, 2019

This because you added bool as implementation constraint:

impl<bool, O: SettingsExt> SettingsExtManual<bool> for O {

try just

impl<O: SettingsExt> SettingsExtManual<bool> for O {
src/prelude.rs Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
@Cogitri Cogitri force-pushed the Cogitri:settingsext branch 3 times, most recently from 742d015 to f3fda09 Jul 6, 2019
src/settings.rs Outdated Show resolved Hide resolved
@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from f3fda09 to ecc92b9 Jul 6, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 6, 2019

Instead of introducing a new trait here, I'd suggest that we fix up the GVariant API so it works closer to how GValue works. And then you can use exactly the same traits (ToVariant / FromVariant) from GVariant here together with g_settings_get_value() / g_settings_set_value().

Makes no sense to replicate the GVariant machinery here another time.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Jul 6, 2019

Ah, so you want to just always use get_value (and converting to the requested type)/set_value (and converting the type to a Variant)? What exactly would need to be fixed in Variant?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 6, 2019

Maybe nothing, is anything missing in GVariant for implementing this here?

In any case, that would be the easiest and in the end equivalent to calling get_boolean(), etc. anyway.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Jul 6, 2019

Maybe nothing, is anything missing in GVariant for implementing this here?

Oh, alright, I'd suggest that we fix up the GVariant API sounded to me as if something needed to be changed :)

Will work on it tomorrow, thanks for the pointer.

@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from ecc92b9 to e092acf Aug 13, 2019
@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 13, 2019

Alright, this now uses the ToVariant/FromVariant API

src/settings.rs Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 13, 2019

Alright, this now uses the ToVariant/FromVariant API

Thanks, looks much nicer now :)

@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from e092acf to 7abb199 Aug 13, 2019
@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 13, 2019

Alright, added Results, will write some tests now to see it in action.

Gir.toml Show resolved Hide resolved
@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from 7abb199 to 63cf937 Aug 13, 2019
@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 13, 2019

Alright, I guess the new version is more in line what GLib expects us to do, since it expects getting an unknown key to be a programmer error. I've made fixup commits so you can see the difference.

@Cogitri Cogitri changed the title settings: add SettingsExtManual Trait for get::<T>/set::<T> fn [WIP] settings: add SettingsExtManual Trait for get::<T>/set::<T> fn Aug 13, 2019
Cogitri added 2 commits Aug 13, 2019
@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from 63cf937 to f2d6381 Aug 14, 2019
build.rs Outdated Show resolved Hide resolved
@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Other than that I think this is good to go now (?)

build.rs Outdated Show resolved Hide resolved
…::<T> fn
@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from f2d6381 to ea6bcbf Aug 14, 2019
@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Hm, the tests seem to be a bit flaky with:

test settings::test::string_get ... 
(process:7244): GLib-GObject-CRITICAL **: 16:50:33.710: g_object_ref: assertion 'G_IS_OBJECT (object)' failed


(process:7244): GLib-GObject-CRITICAL **: 16:50:33.710: g_object_unref: assertion 'G_IS_OBJECT (object)' failed
**
GLib-GObject:ERROR:../gobject/gobject.c:4453:g_weak_ref_set: assertion failed: (weak_locations != NULL)
error: process didn't exit successfully: `/home/rasmus/Projects/gio/target/debug/deps/gio-8e55b7fc5fe6fb99` (signal: 6, SIGABRT: process abort signal)
@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from ea6bcbf to 4c38654 Aug 14, 2019
@Cogitri Cogitri mentioned this pull request Aug 14, 2019
40 of 43 tasks complete
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Can you reproduce the test flakiness yourself too or only on CI? That looks rather problematic, probably memory unsafe somewhere.

I can take a look if it's easy to reproduce. But gdb and valgrind would be the tools of choice here.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

It only happens on my system from time to time while running cargo test

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Alright, seems like it happens when I:

  1. run cargo test
  2. Change something in the source file afterwards (so cargo rebuilds the test)
  3. run cargo test again <- it happens here

Not quite sure why though and it's not consistently happening, but maybe it's just random after all.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Yup, it's random, just run it in a loop until it fails, takes 3-5 tries for me:

#!/bin/bash
cargo test
while [ $? == 0 ]; do
	cargo test
done

Maybe it's because the tests are run in parallel though?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Will take a look. It might be that the GSettings API is not thread-safe and we don't guard against that... but that would be rather unfortunate. I'll take a look :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Also clearly not caused by your changes but something being broken before

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Yes it seems to be caused by running multiple threads at once, awesome. Now to find out if that's a bug in gio or we must prevent usage from multiple threads at once!

src/settings.rs Outdated Show resolved Hide resolved
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

It usually explodes here for me

Thread 1 (Thread 0x7f57fecf5700 (LWP 31753)):
#0  0x00007f57ff504885 in _g_log_abort (breakpoint=1) at ../../../glib/gmessages.c:554
#1  0x00007f57ff505b8d in g_logv (log_domain=0x7f57ff613258 "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7f57fecf41a0) at ../../../glib/gmessages.c:1371
#2  0x00007f57ff505d5f in g_log (log_domain=log_domain@entry=0x7f57ff613258 "GLib-GObject", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7f57ff55557c "%s: assertion '%s' failed") at ../../../glib/gmessages.c:1413
#3  0x00007f57ff506559 in g_return_if_fail_warning (log_domain=log_domain@entry=0x7f57ff613258 "GLib-GObject", pretty_function=pretty_function@entry=0x7f57ff6167f8 <__FUNCTION__.15104> "g_object_ref", expression=expression@entry=0x7f57ff61559f "G_IS_OBJECT (object)") at ../../../glib/gmessages.c:2767
#4  0x00007f57ff5e9c4c in g_object_ref (_object=0x7f57e8002170) at ../../../gobject/gobject.c:3212
#5  0x00007f57ff5efbc5 in g_weak_ref_get (weak_ref=<optimized out>) at ../../../gobject/gobject.c:4393
#6  0x00007f57ff712df2 in g_settings_backend_invoke_closure (user_data=0x7f57e8009e80) at ../../../gio/gsettingsbackend.c:263
#7  0x00007f57ff4ffa05 in g_main_context_invoke_full (context=0x7f57f80061e0, priority=0, function=0x7f57ff712de0 <g_settings_backend_invoke_closure>, data=0x7f57e8009e80, notify=0x0) at ../../../glib/gmain.c:5843
#8  0x00007f57ff4ffa50 in g_main_context_invoke (context=<optimized out>, function=<optimized out>, data=<optimized out>) at ../../../glib/gmain.c:5788
#9  0x00007f57ff712f68 in g_settings_backend_dispatch_signal (backend=0x7f57e8009510 [GMemorySettingsBackend], function_offset=0, name=0x55cc920c0760 "/com/github/gtk-rs/test-bool", origin_tag=0x0, names=0x0) at ../../../gio/gsettingsbackend.c:330
#10 0x00007f57ff7135bb in g_settings_backend_changed (backend=<optimized out>, key=<optimized out>, origin_tag=<optimized out>) at ../../../gio/gsettingsbackend.c:379
#11 0x00007f57ff7129ce in g_memory_settings_backend_write (backend=0x7f57e8009510 [GMemorySettingsBackend], key=0x55cc920c0760 "/com/github/gtk-rs/test-bool", value=0x7f57f8009060, origin_tag=0x0) at ../../../gio/gmemorysettingsbackend.c:80
#12 0x00007f57ff713b24 in g_settings_backend_write (backend=0x7f57e8009510 [GMemorySettingsBackend], key=key@entry=0x55cc920c0760 "/com/github/gtk-rs/test-bool", value=value@entry=0x7f57f8009060, origin_tag=origin_tag@entry=0x0) at ../../../gio/gsettingsbackend.c:793
#13 0x00007f57ff718023 in g_settings_write_to_backend (settings=settings@entry=0x7f57f8007430 [GSettings], value=value@entry=0x7f57f8009060, key=<optimized out>) at ../../../gio/gsettings.c:1151
#14 0x00007f57ff719d78 in g_settings_set_value (settings=0x7f57f8007430 [GSettings], key=0x7f57f80061a0 "test-bool", value=0x7f57f8009060) at ../../../gio/gsettings.c:1591

Thread 2 (Thread 0x7f57f7fff700 (LWP 31755)):
#0  0x00007f57ff48d0df in futex_abstimed_wait (private=0, abstime=0x0, expected=2, futex_word=0x7f57f800b008) at ../sysdeps/unix/sysv/linux/futex-internal.h:172
#1  0x00007f57ff48d0df in __pthread_rwlock_wrlock_full (abstime=0x0, rwlock=0x7f57f800b000) at pthread_rwlock_common.c:803
#2  0x00007f57ff48d0df in __GI___pthread_rwlock_wrlock (rwlock=0x7f57f800b000) at pthread_rwlock_wrlock.c:27
#3  0x00007f57ff5491f5 in g_rw_lock_writer_lock (rw_lock=rw_lock@entry=0x7f57ff62c220 <weak_locations_lock>) at ../../../glib/gthread-posix.c:468
#4  0x00007f57ff5e9f02 in g_object_unref (_object=<optimized out>) at ../../../gobject/gobject.c:3279
--Type <RET> for more, q to quit, c to continue without paging--
#5  0x00007f57ff5e9f02 in g_object_unref (_object=0x7f57f8007eb0) at ../../../gobject/gobject.c:3238

Seems like at least the memory GSettingsBackend is not thread-safe... or maybe even GWeakRef has a thread-safety bug.

@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from 4c38654 to 17a016c Aug 14, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Thread-safety bug in GSettingsBackend. By design it should work but the code is broken.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

See https://gitlab.gnome.org/GNOME/glib/issues/1870 with explanation but I don't know a solution. Until that is fixed and we run a new enough version of GLib on the CI the only solution I see here for now is to disable the tests.

It's all unrelated to your code so I'd suggest to disable the tests and we just go on with life for now.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Wouldn't disabling parallel run for the tests work too?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

True, that's a better idea actually. Let's go for that then :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

You change the travis/appveyor configs to build only with one thread?

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Somehow I had hoped that I could force only one test thread in gio's crate itself so others won't hit this, but seems that that's not possible yet (unless we make our own test harness). I'll modify the configs.

@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Unless we use something like https://crates.io/crates/serial_test

@Cogitri Cogitri force-pushed the Cogitri:settingsext branch from 17a016c to 9266d6e Aug 14, 2019
@Cogitri

This comment has been minimized.

Copy link
Contributor Author

Cogitri commented Aug 14, 2019

Pushed fix

@Cogitri Cogitri changed the title [WIP] settings: add SettingsExtManual Trait for get::<T>/set::<T> fn settings: add SettingsExtManual Trait for get::<T>/set::<T> fn Aug 14, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Nice

@sdroege sdroege merged commit 857145d into gtk-rs:master Aug 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.