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

make read_exact_at cancel-safe #6101

Conversation

problame
Copy link
Contributor

@problame problame commented Dec 11, 2023

Before this PR, we didn't move ownership of the FileGuard to the read future.

This meant the read_exact_at future wasn't cancel-safe, because dropping it while the read future was live could lead to use-after-close of the file descriptor.
We protected against that through a scopeguard.

This PR builds on top of neondatabase/tokio-epoll-uring#27
, passing FileGuard ownership to the tokio-epoll-uring read() operation.

This makes the read_exact_at cancel-safe.

Copy link

github-actions bot commented Dec 11, 2023

No tests were run or test report is not available

Test coverage report is not available

The comment gets automatically updated with the latest test results
53ac78e at 2023-12-12T14:21:15.476Z :recycle:

@problame
Copy link
Contributor Author

Compile error:


error: implementation of `std::marker::Send` is not general enough
  --> pageserver/src/consumption_metrics.rs:61:5
   |
61 | /     task_mgr::spawn(
62 | |         BACKGROUND_RUNTIME.handle(),
63 | |         TaskKind::CalculateSyntheticSize,
64 | |         None,
...  |
77 | |         },
78 | |     );
   | |_____^ implementation of `std::marker::Send` is not general enough
   |
   = note: `std::marker::Send` would have to be implemented for the type `&'0 VirtualFile`, for any lifetime `'0`...
   = note: ...but `std::marker::Send` is actually implemented for the type `&'1 VirtualFile`, for some specific lifetime `'1`
error: higher-ranked lifetime error
    --> pageserver/src/tenant/mgr.rs:2116:5
     |
2116 | /     task_mgr::spawn(
2117 | |         &tokio::runtime::Handle::current(),
2118 | |         TaskKind::GarbageCollector,
2119 | |         Some(tenant_id),
...    |
2164 | |         }
2165 | |     );
     | |_____^
     |
     = note: could not prove `{async block@pageserver/src/tenant/mgr.rs:2123:9: 2164:10}: std::marker::Send`

I think this is rust-lang/rust#102211 (comment) although I freely admit I don't fully understand that issue.

Anyway, following @jcsp 's advice I created a smaller reproducer and found that removing the FileGuard's lifetime generic parameter fixed the problem.

Follow the process in problame/integrate-tokio-epoll-uring/cancel-safe-read-exact-at--2023-12-11-fix-compile-error (the last two commits are interesting)

Pass `FileGuard` to the tokio-epoll-uring read() operation.
and avoids the nasty scopeguard usage.

This makes the `read_exact_at` cancel-safe.

Depends on neondatabase/tokio-epoll-uring#27
@problame problame force-pushed the problame/integrate-tokio-epoll-uring/cancel-safe-read-exact-at branch from 41a2fc2 to 53ac78e Compare December 12, 2023 14:17
@problame problame marked this pull request as ready for review December 12, 2023 14:53
@problame problame requested a review from a team as a code owner December 12, 2023 14:53
@problame problame requested review from jcsp and removed request for a team December 12, 2023 14:53
@problame problame merged commit 53ac78e into problame/integrate-tokio-epoll-uring/wip Dec 12, 2023
17 of 28 checks passed
@problame problame deleted the problame/integrate-tokio-epoll-uring/cancel-safe-read-exact-at branch December 12, 2023 17:37
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.

2 participants