th/no-gasyncresult #227
Closed
th/no-gasyncresult #227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
there are quite some FIXME comments. I may address them still in this branch... |
lkundrak
force-pushed
the
th/no-gasyncresult
branch
2 times, most recently
from
95c4dd0
to
b53c35f
Compare
thom311
force-pushed
the
th/no-gasyncresult
branch
from
b53c35f
to
e1c15d5
Compare
lkundrak
force-pushed
the
th/no-gasyncresult
branch
2 times, most recently
from
f1ca166
to
ff33c79
Compare
Closed
…Result pattern All operations must be cancellable in a timely manner, otherwise, the objects hang during shutdown. Also, get rid of the GAsyncResult pattern. It is more cumbersome than helpful. There are still several issues, marked by FIXME comments.
We should not use GAsyncResult. At least, not for internal API. It's more cumbersome then helpful, in my opinion. It requires this awkward async_finish() pattern. Instead, let the caller pass a suitable callback of the right type.
lkundrak
force-pushed
the
th/no-gasyncresult
branch
from
ff33c79
to
935f808
Compare
Previously nm_ppp_manager_stop() would return a handle which makes it easy to cancel the operation. However, sometimes, we may want to cancel an operation based on an GCancellable. So, extend nm_ppp_manager_stop() to hook it with a cancellable. Essentially, move the code from nm-modem.c to nm-ppp-manager-call.c, where it belongs and where the functionality gets available to every component.
lkundrak
force-pushed
the
th/no-gasyncresult
branch
from
935f808
to
4273430
Compare
- fix stopping ppp-manager. Previously, we would take a reference to priv->ppp_manager to cancel it later. However, deactivate_cleanup() is called first, which already issues nm_ppp_manager_stop(). Thereby, not using a callback and not waiting for the operation to complete. - get rid of this "step" state machine. There are litterally two steps that need to be performed asynchornously. Instead chain the calls. - it is now obviously visible, that the async callback never completes synchronously upon being called (provided that all async operations that it calls themself have this behavior -- which they should).
lkundrak
force-pushed
the
th/no-gasyncresult
branch
from
4273430
to
e61ead5
Compare
|
LGTM |
lkundrak
approved these changes
Oct 17, 2018
There was a problem hiding this comment.
It's more cumbersome then helpful, in my opinion. It requires this awkward async_finish() pattern.
Well; whether it's awkward ot not is purely a matter of taste.
Good thing that you get rid of a handful of GSimpleAsyncResults though. Those are not portable to more recent GLib versions.
|
merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think the
GAsyncResultpattern is less than optimal.Maybe it serves a purpose in public glib API, where it better works for language bindings/introspection. For NetworkManager core, it's just a nuisance.
It results in
GAsyncReadyCallback. Also,g_simple_async_result_set_op_res_gpointer()etc. looses type information).GAsyncResultis itself aGObject, ref-counted and unnecessary "features". In most cases, we need a simple heap allocation to package the user-data and relevant state.nm_utils_user_data_pack()is enough).