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

Fix blocking Akka dispatcher thread on WoT skeleton creation #1666

Merged
merged 7 commits into from
Jun 29, 2023

Conversation

thjaeckle
Copy link
Member

During creating of Things and Features with a given WoT "ThingModel" definition, an API was used which uses CompletableFuture and which does blocking calls (e.g. in order to retrieve the model(s) via HTTP).

This however was directly called via the Akka "dispatcher" thread - so other processing was completely halted, even the Actor system of the things node could be affected by not being able to schedule any actor related work.

This bugfix enables the internal mechanisms of Ditto to be also able to deal with asynchronously provided events to persist - which will offload the blocking operations to the already provided "wot-dispatcher" instead.

…r thread

* whenever creating a Thing using the WoT "ThingModel" included in the to-be-created thing a `.join()` of a CompletableFuture is called on the main Akka dispatcher thread
* this causes that the complete ActorSystem might be blocked to process any futher messages
* fixed that by making ResultVisitor work with a CompletionStage instead

Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io>
@thjaeckle thjaeckle added this to the 3.3.1 milestone Jun 26, 2023
@thjaeckle thjaeckle added the bug label Jun 26, 2023
@thjaeckle
Copy link
Member Author

@kalinkostashki @alstanchev this is quite important for me.
Could you squeeze in a review and a system test run?

@alstanchev
Copy link
Contributor

Sure will do my best. Will look in to it later today.

Copy link
Contributor

@alstanchev alstanchev left a comment

Choose a reason for hiding this comment

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

I have left few comments in the code other then that looks good.
Tests are running currently had one failure in the Things unit tests which i believe is from the thing i have commented. For the sake of time i tried a local fix and am currently waiting on the builds.

…xceptionally" instead on CompletionStage

Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io>
@thjaeckle
Copy link
Member Author

thjaeckle commented Jun 27, 2023

@alstanchev I am still working on this as I encountered some more .join() calls which were done on the Akka main dispatcher thread.
I tried to replace them all with CompletionStage based APIs, I am however stuck when persisting in the AbstractPersistenceActor - there I have an open "TODO" to replace the explicit blocking.

Without that however the event is not persisted correctly (in case of WoT based Thing creations) ..

So still working on it :)

I however fixed the PolicyImportsIT#putPolicyWithTooManyImports() test

* quite a refactoring was needed overall
* still with an open "TODO" as I got not rid of blocking on the Akka dispatcher thread when persisting a "CreateThing"

Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io>
@thjaeckle
Copy link
Member Author

@alstanchev I am confident that I solved it now ..

I reduced the potential blocking on the Akka dispatcher thread to a minimum - as I did not manage to get the "persist" to work without it.

Let me sleep one night, maybe I have a better idea tomorrow :)

* only for specific commands, enable an "async path" for mutations and queries
* still block on the Akka dispatcher thread, otherwise the persist will not work

Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io>
@thjaeckle thjaeckle force-pushed the bugfix/blocking-on-wot-skeleton-creation branch from 1fc2aaa to 50202df Compare June 27, 2023 21:01
@thjaeckle
Copy link
Member Author

@alstanchev this morning I got rid of blocking on the Akka dispatcher thread completely 😁
With that I would say it is ready to be reviewed.

Sorry that it is that many changes, I completely made the WoT integration async - I don't know why I did not do that when creating it in the first place.. :/

Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io>
@thjaeckle thjaeckle self-assigned this Jun 28, 2023
@thjaeckle
Copy link
Member Author

thjaeckle commented Jun 28, 2023

@alstanchev there is one unit test failing on GitHub action build - locally however this work both when starting in the IDE and when building via maven.

As you are the author of this test, maybe do you have a clue why it would fail?
ThingPersistenceActorTest.policyShouldNotBeDeletedOnThingRetrieveAndActorFail

One hint: I changed something in the "rollback created policy logic when thing creation fails" as this was IMO not done correctly:

.exceptionallyCompose(error -> handleTargetActorAndEnforcerException(transformed, error))

Here I replaced signal with transformed - as an incoming ModifyThing command could be transformed to a CreateThing by the handleOptionalTransformationException method when the Thing did not yet exist before.
In such a case, the rollback of the policy was not working..

As the unit test tests something in that direction, maybe this is related?

Edit: in the last run, the test was successful

Copy link
Contributor

@alstanchev alstanchev left a comment

Choose a reason for hiding this comment

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

LGTM ;)

@thjaeckle thjaeckle merged commit ce30559 into master Jun 29, 2023
2 checks passed
@thjaeckle thjaeckle deleted the bugfix/blocking-on-wot-skeleton-creation branch June 29, 2023 17:45
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants