Skip to content

Support punchthrough tokens that have lifetime restrictions#9

Closed
jaybosamiya-ms wants to merge 1 commit intomainfrom
jayb/push-yvmnxmyxnkty
Closed

Support punchthrough tokens that have lifetime restrictions#9
jaybosamiya-ms wants to merge 1 commit intomainfrom
jayb/push-yvmnxmyxnkty

Conversation

@jaybosamiya-ms
Copy link
Copy Markdown
Member

This PR adds a generic lifetime bound to the associated type for the PunchthroughToken, which allows us to more precisely capture lifetimes, and thereby de-restrict the conditions under which punchthrough tokens can be implemented (such as those with explicit lifetimes within them).

@jaybosamiya-ms jaybosamiya-ms requested a review from CvvT February 12, 2025 01:02
@jaybosamiya-ms jaybosamiya-ms mentioned this pull request Feb 12, 2025
Closed
@jaybosamiya-ms
Copy link
Copy Markdown
Member Author

Ah, it looks like this particular feature breaks dyn-compatibility (thus the semver checks complaint). I'll have to think a little bit if it is possible to maintain dyn compatibility while also supporting the restricted lifetimes we want to support. I suspect it is not possible (we need to give up one or the other).

In general, I don't expect us to ever want to have multiple runtime-generic platforms that are fully-runtime-dynamic-dispatched. Thus, I don't think we actually need dyn-compatibility. Indeed, we would still be able to support multiple static platforms at the same time if we wanted to, so this is not a major restriction. Thus, I feel we can give up on dyn compatibility here.

Copy link
Copy Markdown
Member

@wdcui wdcui left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@CvvT
Copy link
Copy Markdown
Contributor

CvvT commented Feb 12, 2025

As discussed with Jay this afternoon, the current design assumes that punchthrough does not block. One example of blocking punchthrough is futex (might be the only one). Either we remove mut from get_punchthrough_token_for so we can get multiple tokens from the provider concurrently, or we consider futex is a special case that should be handled separately.

@wdcui
Copy link
Copy Markdown
Member

wdcui commented Feb 12, 2025

What's the downside of just removing mut?

As discussed with Jay this afternoon, the current design assumes that punchthrough does not block. One example of blocking punchthrough is futex (might be the only one). Either we remove mut from get_punchthrough_token_for so we can get multiple tokens from the provider concurrently, or we consider futex is a special case that should be handled separately.

@CvvT
Copy link
Copy Markdown
Contributor

CvvT commented Feb 12, 2025

Ah, it looks like this particular feature breaks dyn-compatibility (thus the semver checks complaint). I'll have to think a little bit if it is possible to maintain dyn compatibility while also supporting the restricted lifetimes we want to support. I suspect it is not possible (we need to give up one or the other).

In general, I don't expect us to ever want to have multiple runtime-generic platforms that are fully-runtime-dynamic-dispatched. Thus, I don't think we actually need dyn-compatibility. Indeed, we would still be able to support multiple static platforms at the same time if we wanted to, so this is not a major restriction. Thus, I feel we can give up on dyn compatibility here.

I drafted a new PR: #10. I removed most punchthroughs and only kept syscalls, now it does not need to specify lifetime.

@CvvT
Copy link
Copy Markdown
Contributor

CvvT commented Feb 12, 2025

What's the downside of just removing mut?

As discussed with Jay this afternoon, the current design assumes that punchthrough does not block. One example of blocking punchthrough is futex (might be the only one). Either we remove mut from get_punchthrough_token_for so we can get multiple tokens from the provider concurrently, or we consider futex is a special case that should be handled separately.

Jay just told me futex could be supported in litebox given RawMutexProvider implementation and our single process assumption. Probably we won't have any blocking punchthroughs.

@CvvT CvvT mentioned this pull request Feb 12, 2025
@jaybosamiya-ms
Copy link
Copy Markdown
Member Author

I'm closing this PR, since it appears that we don't need it. If necessary, we can resurrect it.

Keeping the mut around helps us be aware of situations where we might be using punchthroughs "badly". It is more of a "if you can get this to work with the mut then it is more likely you got the implementation correct; if we remove the mut then it is easier to make mistakes". Both with and without mut could be correct (thus the more expressive approach would be without mut), but I personally feel it is better to keep things restrictive until we have a good reason to derestrict it.

@jaybosamiya-ms jaybosamiya-ms deleted the jayb/push-yvmnxmyxnkty branch February 13, 2025 22:10
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.

3 participants