chore(state-machine): Panic in the case of non-handled non-deferred e… #1464

oleorhagen · 2023-11-01T12:43:05Z

No description provided.

mender-test-bot · 2023-11-01T12:43:36Z

@oleorhagen, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

my commands and options

You can trigger a pipeline on multiple prs with: - mentioning me and `start pipeline --pr mender/127 --pr mender-connect/255`

You can start a fast pipeline, disabling full integration tests with:

mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

mentioning me and sync

You can cherry pick to a given branch or branches with:

mentioning me and:

 cherry-pick to:
 * 1.0.x
 * 2.0.x

ghost

Good safeguard!

ghost · 2023-11-03T06:43:05Z

src/common/state_machine.hpp

+
+		// Check if there are any non-deferred events in the queue - then fail if
+		queue<EventType> queue_copy = event_queue_;
+		while (not queue_copy.empty()) {


It's expected to have non-deferred events here, because this is after the handlers have run. I think the check needs to be between the if (!to_run.empty()) { block and the one above it.

There is also a much easier way to check this. When we are doing event_queue_.push(event) above, also increment a deferred_count counter. If deferred_count < event_queue_.size(), then the check is positive.

Oh! Very good idea!

I had a suspicion you would come up with something smarter than me here 😆

Alright, new try. I am still using my original approach, but moved it to before we run the machine loop.

Ur approach, although clever, did not work when initializing (since we add the start events to the to_run queue, and I didn't feel like special casing it.

This can be easily fixed too. Just keep a counter for the run_queue as well, but don't increment it during the initialization step, only when adding events in the "normal" flow. Then the check becomes deferred_count < added_to_run_queue. I think it's better than copying the whole queue on every state transition.

True, I did dabble with the idea. But decided it's not really better. This implementation reads pretty straight forward, and does not have to touch the other pieces of code, and handle special cases.

Copying the queue should just be a couple of elements

Ok then, I'll let it go! 🙂

ghost · 2023-12-04T07:23:23Z

src/common/dbus/platform/asio_libdbus/dbus.cpp

-	DBusError dbus_error;
-	dbus_error_init(&dbus_error);
-	dbus_conn_.reset(dbus_bus_get_private(DBUS_BUS_SYSTEM, &dbus_error));
+	std::unique_ptr<DBusError, void (*)(DBusError *)> dbus_error {new DBusError, dbus_error_free};


I support the incentive, but I suspect you have in fact have introduced a memory leak here. There are two levels of resources here, the DBusError struct itself, and the content it contains. dbus_error_free only frees the latter.

I can see two ways to solve it:

Use a lambda with both dbus_error_free and delete dbus_error in it.

Don't use new DBusError, but keep the variable on the stack instead, as before, and use &dbus_error as the argument when constructing the smart pointer (with a different name), together with dbus_error_free. The unique_ptr now doesn't own the pointer itself, but it owns the contents.

Same for the other commit.

Oh shite. True. Rookie mistake!

ghost · 2023-12-04T07:55:09Z

src/common/state_machine.hpp

+
+		// Check if there are any non-deferred events in the queue - then fail if
+		queue<EventType> queue_copy = event_queue_;
+		while (not queue_copy.empty()) {


This can be easily fixed too. Just keep a counter for the run_queue as well, but don't increment it during the initialization step, only when adding events in the "normal" flow. Then the check becomes deferred_count < added_to_run_queue. I think it's better than copying the whole queue on every state transition.

Just makes it more future proof, as we don't have to remember free it in our exit paths anylonger. Signed-off-by: Ole Petter <[email protected]>

This just makes this consistent with the rest of our codebase. Signed-off-by: Ole Petter <[email protected]>

Signed-off-by: Ole Petter <[email protected]>

Simply split out and name the logical parts. Signed-off-by: Ole Petter <[email protected]>

…vents This adds a sanity check to verify that there are no non-deferred events left in the event queue after the run_queue has been populated. If there was, then we would have undefined behaviour in the state-machine, and as such the best thing we can do in this instance is to panic. This is a serious logic error in our code, and as such such cause a hard-fault. Signed-off-by: Ole Petter <[email protected]>

ghost · 2023-12-05T07:18:06Z

src/common/dbus/platform/asio_libdbus/dbus.cpp

-				+ dbus_error.name + "]");
-		dbus_error_free(&dbus_error);
+			string("Failed to get connection to system bus: ") + dbus_error.get()->message + "["
+				+ dbus_error.get()->name + "]");


.get() should only be used when it's required to, here it can be omitted.

stale · 2024-03-17T13:45:33Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

lluiscampos requested a review from a user November 2, 2023 14:02

ghost suggested changes Nov 3, 2023

View reviewed changes

oleorhagen force-pushed the state-machine-non-deferred-events branch from 8966992 to 5f8c974 Compare November 30, 2023 13:37

oleorhagen requested a review from a user November 30, 2023 13:39

ghost suggested changes Dec 4, 2023

View reviewed changes

oleorhagen added 5 commits December 4, 2023 13:46

refac(dbus): Wrap the DbusError resource in a smart pointer

72bcf04

Just makes it more future proof, as we don't have to remember free it in our exit paths anylonger. Signed-off-by: Ole Petter <[email protected]>

refac(dbus): Replace all NULL instances with nullptr

1548f54

This just makes this consistent with the rest of our codebase. Signed-off-by: Ole Petter <[email protected]>

chore: Spelling 'A sync' -> 'Async'

000e106

Signed-off-by: Ole Petter <[email protected]>

refac(state-machine): Clarify the logic in the RunOne function

d21b262

Simply split out and name the logical parts. Signed-off-by: Ole Petter <[email protected]>

oleorhagen force-pushed the state-machine-non-deferred-events branch from 5f8c974 to 9cad8e7 Compare December 4, 2023 12:47

ghost approved these changes Dec 5, 2023

View reviewed changes

ghost mentioned this pull request Dec 8, 2023

MEN-6836: Do not call the user's callback when returning an error #1531

Merged

stale bot added the wontfix label Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(state-machine): Panic in the case of non-handled non-deferred e… #1464

chore(state-machine): Panic in the case of non-handled non-deferred e… #1464

oleorhagen commented Nov 1, 2023

mender-test-bot commented Nov 1, 2023

ghost left a comment

ghost Nov 3, 2023

oleorhagen Nov 3, 2023

oleorhagen Nov 30, 2023

ghost Dec 4, 2023

oleorhagen Dec 4, 2023

oleorhagen Dec 4, 2023

ghost Dec 4, 2023

ghost Dec 4, 2023

oleorhagen Dec 4, 2023

ghost Dec 4, 2023

ghost Dec 5, 2023

stale bot commented Mar 17, 2024

chore(state-machine): Panic in the case of non-handled non-deferred e… #1464

Are you sure you want to change the base?

chore(state-machine): Panic in the case of non-handled non-deferred e… #1464

Conversation

oleorhagen commented Nov 1, 2023

mender-test-bot commented Nov 1, 2023

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Mar 17, 2024