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

Introduce AutoHCK::ResourceScope #227

Merged
merged 11 commits into from
Aug 13, 2023
Merged

Introduce AutoHCK::ResourceScope #227

merged 11 commits into from
Aug 13, 2023

Conversation

akihikodaki
Copy link
Contributor

I have already done several changes to improve the abortion logic, but I still see an error occurs during abortion. I also heard AutoHCK sometimes hangs after the tests finish, which imply some resources associated threads are left unreleased.

To eliminate this kind of errors completely, I propose AutoHCK::ResourceScope as a single true resource management mechanism.

This pull request makes every code dealing with unmanaged resources use the new mechanism so it's huge. Take time if it's correctly used everywhere before merging.

@akihikodaki akihikodaki requested a review from kostyanf14 July 12, 2023 09:54
@akihikodaki
Copy link
Contributor Author

Also: I'm still testing and have not confirmed its well-behaving.

Copy link
Contributor

@kostyanf14 kostyanf14 left a comment

Choose a reason for hiding this comment

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

Very good job. Have several questions for now. Will be back after testing.

lib/setupmanagers/hckclient.rb Outdated Show resolved Hide resolved
lib/setupmanagers/hckclient.rb Outdated Show resolved Hide resolved
lib/engines/hcktest/hcktest.rb Show resolved Hide resolved
@akihikodaki akihikodaki force-pushed the akihikodaki/scope branch 2 times, most recently from a7a3397 to dee06ea Compare July 12, 2023 10:53
@akihikodaki akihikodaki force-pushed the akihikodaki/scope branch 2 times, most recently from 5480222 to 0ff4aa5 Compare July 12, 2023 11:04
@akihikodaki akihikodaki requested a review from kostyanf14 July 12, 2023 11:08
@akihikodaki akihikodaki force-pushed the akihikodaki/scope branch 3 times, most recently from 546fa68 to b561ef5 Compare July 13, 2023 10:21
@akihikodaki
Copy link
Contributor Author

It happened to hang with old Timeout so I fixed it with commit 1b276f3.
It did not hang with a recent Timeout because of a regression. For details of the regression, see: ruby/timeout#41

@akihikodaki
Copy link
Contributor Author

Commit 4b59a08 removed destructive methods from AutoHCK::ResourceScope so that you can pass its instances around without worrying that resources already allocated may be destructed somewhere you are unaware of.

This ensures that the file gets closed even when an error raised
during file operation.

Signed-off-by: Akihiko Odaki <[email protected]>
This saves lots of lines and aids following refactorings.

Signed-off-by: Akihiko Odaki <[email protected]>
Before this change, studio or client was chosen according to the name
parameter. However, the values of the name parameter were not defined
in one place. Particularly, the name of studio machine was defined in
two places, AutoHCK::HCKStudio::STUDIO and AutoHCK::QemuHCK::STUDIO.
Eliminate this potential hazard by introducing specialized methods for
running studio or client machines.

Signed-off-by: Akihiko Odaki <[email protected]>
It is a common idiom in Ruby to perform operating system calls during
object initialization to implement the
Resource-Acquisition-Is-Initialization pattern and to gain its benefit.
One of such an existing example is File class, which performs file
open operation during initialization; that clarifies the close method
must be called when an instance exists.

Signed-off-by: Akihiko Odaki <[email protected]>
Managing a process from several threads can lead to race conditions;
we already have some code to deal with such a race condition that can
happen when a process reap is occuring. Perform all process management
operations in a dedicated thread to avoid them.

Signed-off-by: Akihiko Odaki <[email protected]>
Before this change, the management of resources that require explicit
deallocations were done ad-hoc and inconsistent. This new class
provides a unified mechanism for this purpose and ensures the following:
- Resource deallocation happens in reverse-allocation order.
  This is important because an object often depend on another object
  allocated earlier and cannot outlive the earlier object.
- Interrupts during deallocation will be postponed.
  (Ensured with Thread.handle_interrupt(AutoHCKInterrupt => :never))
- An interrupt won't be handled while an allocated object is assigned to
  a AutoHCK::ResourceScope.
  (Ensured with
   Thread.handle_interrupt(AutoHCKInterrupt => :on_blocking))

For Thread.handle_interrupt invocation, a dedicated exception class,
AutoHCKInterrupt is used so that it will not interfer with other
interrupts that can happen in child threads.

Moreover, this class provides transaction feature that facilitates error
handling during resource allocation.  If an error happens in a
transaction, resources assigned to the transaction scope will be safely
deallocated. Otherwise, they will escape the transaction scope and be
available to the outer scope.

Signed-off-by: Akihiko Odaki <[email protected]>
HCK is not yet available during installation so avoid using it.

Signed-off-by: Akihiko Odaki <[email protected]>
This allows to react quickly when QEMU finishes.

Signed-off-by: Akihiko Odaki <[email protected]>
The test configuration step is quite a complicated; it first launches
machines, and performs various configuration steps. To provide more
visibility, log when machines are being launched.

Signed-off-by: Akihiko Odaki <[email protected]>
The message is printed for normal exit as well so the word "aborting"
is inappropriate.

Signed-off-by: Akihiko Odaki <[email protected]>
@YanVugenfirer YanVugenfirer merged commit 349ba85 into master Aug 13, 2023
@YanVugenfirer YanVugenfirer deleted the akihikodaki/scope branch August 13, 2023 08:41
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.

3 participants