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

Concurrency issue at analyze #3907

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Conversation

rscampos
Copy link
Collaborator

@rscampos rscampos commented Mar 5, 2024

1. Explain what the PR does

fix: #3903
fix: #3905

Instead of terminating the program upon reaching the EOF (after all events have been read), ensure the program concludes only after all events have been fully processed.

2. Explain how to test it

3. Other comments

- produce close the engineInput instead of close producerFinished,
in this way the engine knows when the events finishes

- add a close in the function consumeSources, in this way the
consumer knows when Finding finishes
@rscampos rscampos force-pushed the analyze_concurrency branch from 311ae37 to 5b5426e Compare March 6, 2024 19:13
@geyslan
Copy link
Member

geyslan commented Mar 6, 2024

Going to review this now. 👍🏼

@geyslan
Copy link
Member

geyslan commented Mar 6, 2024

The issues are reproducible indeed.

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM.

Tested with a huge json (100000000 entries / 109G) as input and analyze processed all events accordingly.

Signalling Tracee to finish during the analyze process worked smoothly as well.

pkg/signatures/engine/engine.go Show resolved Hide resolved
Copy link
Contributor

@AlonZivony AlonZivony left a comment

Choose a reason for hiding this comment

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

Small thing to add on the same note.
Overall LGTM

cmd/tracee/cmd/analyze.go Outdated Show resolved Hide resolved
@rscampos rscampos requested a review from AlonZivony March 21, 2024 13:07
@rscampos rscampos force-pushed the analyze_concurrency branch from 50d48d2 to e879b11 Compare March 21, 2024 14:36
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM

@geyslan geyslan dismissed AlonZivony’s stale review March 21, 2024 19:36

It was already done.

@geyslan geyslan merged commit 544d710 into aquasecurity:main Mar 21, 2024
32 checks passed
@geyslan
Copy link
Member

geyslan commented Mar 21, 2024

Many thanks @rscampos for your help on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants