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

onReady seems to be a private member, how can i use it #239

Open
shuyanzhu opened this issue Jun 5, 2023 · 8 comments
Open

onReady seems to be a private member, how can i use it #239

shuyanzhu opened this issue Jun 5, 2023 · 8 comments

Comments

@shuyanzhu
Copy link
Author

shuyanzhu commented Jun 5, 2023

I may figure out why onReady is private. The docment should be updated before onRead is available?

// TODO: T87126824. The behavior for (2) is far from ideal. CacheLib will

@therealgymmy
Copy link
Contributor

@shuyanzhu: yeah you're right. We have actually changed our thinking since the comment is written. We now think everyone should convert a "not-ready" item-handle to a SemiFuture as that's a pretty standard way of dealing with async now (and also plays well with folly's implementation of coroutine)

we'll update the documentation

facebook-github-bot pushed a commit that referenced this issue Jun 5, 2023
Summary:
We no longer let users directly attach a callback to onReady(), which is unsafe due to thread handle count.

This caused some confusion in the PR: #239

This diff cleans up the comments

Reviewed By: jaesoo-fb

Differential Revision: D46446698

fbshipit-source-id: fb26a7a731a80cf7f5158b8f2ebaa261bb72b62b
@shuyanzhu
Copy link
Author

shuyanzhu commented Jun 6, 2023

I use bthread in my code, which is stackful coroutine like folly/fiber. In such cases, i think onReady is needed beacause folly::SemiFuture::get() would block the pthread of coroutine. I want use onReady to call my notify callback(cv.notify()) so i can suspend coroutine correctly(cv.wait()).

@therealgymmy
Copy link
Contributor

@shuyanzhu: I suppose you cannot use folly coroutine? You can hack it for now by making onReady() public on your end, and adjust the thread local count yourself for now.

Are you doing this for a production system, or for research?

@shuyanzhu
Copy link
Author

shuyanzhu commented Jun 6, 2023

I use cachelib in a production system so i cannot use folly coroutine. Yeah, i have made onRead public.

@shuyanzhu
Copy link
Author

shuyanzhu commented Jun 6, 2023

BTW, i can also new a excutor to complete folly::SemiFuture which had defered bthread's cv.notify, but it seems to introduce unnecessary overhead.

@shuyanzhu
Copy link
Author

@therealgymmy I wonder if 'onReady' will be re-exposed to users.

@therealgymmy
Copy link
Contributor

@shuyanzhu: yeah I filed a ticket for it. We'll think about a better way to expose this. You're right that not everyone will be using folly's version of executors.

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

No branches or pull requests

2 participants