Refactor - Adds check that only loaded programs can be unloaded #35146
Conversation
Lichtso
force-pushed
the
refactor/loaded_program_to_unloaded
branch
from
54af127 to
174023e
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #35146 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 833 833
Lines 224827 224830 +3
=======================================
+ Hits 183523 183598 +75
+ Misses 41304 41232 -72 |
Lichtso
force-pushed
the
refactor/loaded_program_to_unloaded
branch
4 times, most recently
from
8fd99d1 to
62b6d29
Compare
Lichtso
force-pushed
the
refactor/loaded_program_to_unloaded
branch
2 times, most recently
from
e6ef4ec to
6491692
Compare
| @@ -2386,6 +2375,38 @@ mod tests { | |||
| assert!(match_missing(&missing, &program3, true)); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_unloaded() { | |||
There was a problem hiding this comment.
This looks good, but we still don't have tests for the None path in
sort_and_unload and evict_using_2s_random_selection?
Lichtso
force-pushed
the
refactor/loaded_program_to_unloaded
branch
from
6491692 to
1c46990
Compare
| for loaded_program_type in [ | ||
| LoadedProgramType::FailedVerification(cache.environments.program_runtime_v1.clone()), | ||
| LoadedProgramType::Closed, | ||
| LoadedProgramType::DelayVisibility, |
There was a problem hiding this comment.
This can't go in the cache right? I would comment it and explicitly say that. It's weird if tests test things which aren't supposed to happen because then you can't use tests to learn an API.
| let program_id = Pubkey::new_unique(); | ||
| cache.assign_program(program_id, entry.clone()); | ||
| cache.unload_program_entry(&program_id, &entry); | ||
| assert_eq!( |
There was a problem hiding this comment.
I think we should check that stats.evictions is 1 here?
There was a problem hiding this comment.
evictions only increments on successful unloading, not for attempts.
There was a problem hiding this comment.
So we should check that it goes up when expected? We must check both paths of to_unloaded somehow
Lichtso
force-pushed
the
refactor/loaded_program_to_unloaded
branch
from
1c46990 to
42c5aa0
Compare
Lichtso
force-pushed
the
refactor/loaded_program_to_unloaded
branch
from
42c5aa0 to
6943b44
Compare
Problem
Currently the only thing stopping a tombstone from being unloaded is that it never gets selected as a candidate by
LoadedPrograms::get_flattened_entries().Summary of Changes
Adds check that prevents anything but loaded programs from being unloaded.