fix panic when doing Ctrl-C in the middle of a attack #26
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
=======================================
Coverage 77.59% 77.59%
=======================================
Files 7 7
Lines 281 281
=======================================
Hits 218 218
Misses 45 45
Partials 18 18
Continue to review full report at Codecov.
|
| @@ -95,7 +95,4 @@ func Attack(ctx context.Context, target string, resCh chan *Result, metricsCh ch | |||
| } | |||
| } | |||
| metrics.Close() | |||
There was a problem hiding this comment.
To show metrics that cannot be measured until the attack is completed, we'd better send after closing metrics.
| metrics.Close() | |
| metrics.Close() | |
| metricsCh <- newMetrics(&metrics) |
There was a problem hiding this comment.
That actually makes a lot of sense. Will get to work on it
|
The root problem can be solved by removing the |
|
@brenol Thanks for your contribution! It's a great help with multiple issues. Let me just make one thing clear. Your commit and your Github account don't seem to be tied to each other: I'd like to welcome you as a contributor, but as it stands, you don't count as a contributor. Would it be possible to authenticate the Github account on your local PC? |
Sure, I'll. I knew removing the close() would fix the issue (and it was the first thing I did) but I did it right when I started navigating through the codebase, so I wasn't sure if it would be actually correct. I'll get that github thingie fixed asap too. |
|
Flaky test is hard. Thought I could fix it as-is but I wasn't unable to >sadface< . I have pushed what I believe is a middleground, but it just feels wrong. If you don't have any problem with the test that is failing, I'll undo the middleground. |
| @@ -88,6 +88,8 @@ func Attack(ctx context.Context, target string, resCh chan *Result, metricsCh ch | |||
| select { | |||
| case <-ctx.Done(): | |||
| opts.Attacker.Stop() | |||
| // metricsCh is already closed (as context is done) so we shouldn't send any metric | |||
| return | |||
There was a problem hiding this comment.
Nice idea! no problem to give back the function when the context expired.
gui/drawer.go
Outdated
| @@ -103,6 +103,7 @@ End: %v` | |||
| ) | |||
|
|
|||
| func (d *drawer) redrawMetrics(ctx context.Context) { | |||
| defer close(d.metricsCh) | |||
There was a problem hiding this comment.
To be honest, we want to get rid of it. But let us leave it as is for once.
|
@brenol Thank you for taking the time to solve the problem! LTGM so let me merge this once Github account stuff solved. |
|
Still not solved? Thought I was able to solve it (set .gitconfig e-mail to my no-reply e-mail; seems I'm still missing something) |
…nd do not send any metric after we're done from the attacker
brenol
force-pushed
the
fix-panic-when-stopping-infinite-attack
branch
from
ea607f7
to
95a3d3f
Compare
|
Now it's correct, right? Sorry for the force push, easiest way to fix it :) |
this should fix #23.
As the Attack method is now sending messages to the 'metricsCh', and it looks at the Context being complete (completely stopping the attack), I simply removed the channel send that was being done to metricsCh in gui/keybinds.go . This fixed the panic and the Ctrl-C issue for me.