WIP: Convert actix-service to use RefCell instead of UnsafeCell

[dunnock]

Hi, this is first PoC implementation for gathering feedback to make sure I am on the right path.

For smooth conversion I have added AXCell, which is similar to former Cell but using RefCell under the hood. Also added couple of concurrency tests with delays on first and second service to validate that there are no 2 mutable borrows which would lead to panic with RefCell.

By benchmarks pipeline with new AndThen performing similar to UnsafeCell baseline implementation:

AndThen with UnsafeCell #2              time:   [54.157 ns 54.850 ns 55.605 ns]
AndThen with RefCell #2                 time:   [54.976 ns 55.624 ns 56.353 ns]                                    
Pipeline::and_then based Rc<RefCell>    time:   [54.696 ns 55.260 ns 55.866 ns]

Pls let me know if using this approach is ok?

[dunnock]

FYI just submit issue about windows mingw build: numworks/setup-msys2#29

[JohnTitor]

Disabled mingw builders temporarily on master.

[cdbattags]

I'm good with this approach.

Might be nice to encapsulate this module elsewhere if we do decide to go this way.

[cetra3]

Looks good! I like the abstraction of AXCell but think the name might be a little confusing (I am definitely bikeshedding though!).

[Shnatsel]

This not only removes unsafe code, this is also an important soundness fix. Is there anything that's holding up the merge? It'd be nice to get this landed and released sooner rather than later.

The approach looks good to me. Rc<RefCell<T>> is indeed how Actix originally functioned, before the introduction of the custom cell: 20b03a4

[robjtede]

Is there anything that's holding up the merge?

It’s still in draft and needs some perf measurements done so we can know the impact of any regression.

Also, my focus is on v3 release blockers which this isn’t. It could be a patch release later.

[dunnock]

Sorry for delaying on this, I was dragged in some other works, but will try to push this further during next week.
Do you know why RefCell was changed to UnsafeCell in the first place?

[Shnatsel]

Do you know why RefCell was changed to UnsafeCell in the first place?

No. The commit message merely reads "add custom cell".

I suspect the reason was performance - I believe it is possible to increment one counter instead of two on every access, if you place certain additional restrictions on the usage patterns. I believe this was the intent, although I cannot know for sure.

I do not expect this to have any meaningful effect on performance outside of microbenchmarks. The counters are not atomic and are placed on the same L1 cache line. The cost of performing two increments instead of one should be so trivial as not to be worth considering compared to even an L2 cache access. Or that's the theory, at least.

[codecov]

Codecov Report

Merging #113 into master will increase coverage by 0.14%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   61.59%   61.73%   +0.14%     
==========================================
  Files          80       81       +1     
  Lines        5028     5070      +42     
==========================================
+ Hits         3097     3130      +33     
- Misses       1931     1940       +9     

Impacted Files	Coverage Δ
actix-service/benches/and_then.rs	0.00% <ø> (ø)
actix-service/src/lib.rs	42.42% <ø> (ø)
actix-service/src/rcrefcell.rs	50.00% <50.00%> (ø)
actix-service/src/and_then.rs	90.90% <90.90%> (-0.35%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7140c04...a37c3fb. Read the comment docs.

[Shnatsel]

@dunnock this looks complete to me, any reason to keep this in draft?

This fixes a long-standing memory safety issue that allows obtaining several mutable references to the same data and get arbitrary memory corruption from there (PoC that fails MIRI). Moreover, benchmarks demonstrating lack of performance impact are included. I'm surprised that 6 months later this is still not merged.

[robjtede]

Adding this to v3 blockers list so I don't forget about it again.

[dunnock]

Last time I've tried to convert wekk ago I run into issues with AndThenApplyFn, probably that was the reason why the Cell was introduced in the first place..
since b is RefMut now, not T as before, it does not let to destructure tuple in the following code:

                    let mut b = b.get_mut();
                    let fut = (&mut b.2)(res, &mut b.1);

Probably will need to split that tuple in the first place...

[Shnatsel]

There is a simpler patch to fix the soundness holes that replaces all occurrences of Actix cell with Rc<RefCell<T>>, the exact same thing that was used before the introduction of the custom cell. It passed tests when it was written (same age as this PR).

Any reason not to go with that right now to fix the memory issue? We can always think of a fancier solution later.

[robjtede]

Closing this since #158 merged. Appreciate everyones input on this important fix.

[dunnock]

Cool, thanks for taking lead on this!