ivmarkov
commented
1 month ago I had to take the RefCell out, because wait() would await holding &RefCell, making the futures require Sync.
It was replaced by a Mutex, since the socket read/write splitting design is not clear yet.
First: a diversion regarding borrowing/locking across await points:
I don't think we are actually keeping a borrow of the RefCell across await points. Clippy would've complained if that's the case, and really, where do you see that? Also of note - putting a blocking Mutex (by the way - if you need a blocking mutex and you want to stay no_std compatible, you can use the one in esp_idf_svc::private::mutex) does not solve this problem (need to keep a lock during awaits) but makes it worse of course, as now you might have deadlocks instead of panics, in data race cases.
Even an async Mutex won't solve the "issue" in our particular case, precisely because of socket splitting: putting an async mutex around EspTls would prevent the reader and writer from working concurrently, as the first one to enter the async mutex (say, the reader) would prevent the other one (say, the writer) to poll for "I can write" until the reader exits the async mutex.
Really, the only proper way to do locks in the presence of socket splitting is to use RefCell borrows (single-threaded) or blocking mutex locks (multi-threaded) and ensure you don't keep the lock/borrow across await points.
Regardless, the above issue is I hope moot, simply because I don't believe we are holding a lock (a borrow, actually) across await points in the first place! We only do ii for a short glimpse when reading/writing data from/to the socket, when data is already reported to be available, as it should be.
Second: why I think UnsafeCell is not a solution and might blow up (please read carefully, I might be wrong! :) ):
A multi-threaded executor wants futures to be Send, so that it can do the first poll of the future on one thread, then the second poll of the future on potentially a different thread and so on.
Looking at the signature of - say - async fn read(&self, buf: &mut [u8]) -> Result<..> - this is just sugar for
fn read<'t>(&'t self, buf: &mut [u8]) -> impl Future<Output = Result<..>> + 't (where 't is very important).
So if the future needs to be Send, then &'t self needs to be Send too, as it is contained inside the future (that's why the impl Future<...> has + 't). But then if &'t self needs to be Send, then self itself needs to be Sync! Hence you actually DO need a blocking mutex around EspTls.
A concrete example why EspTls needs to be mutexed (and thus Sync) would be a "read" future running in one thread, and then a "write" future running in another. In that case, they both need concurrent access to EspTls, hence access to EspTls needs to be synchrionized.
Please let me know if you see gaps in my thinking!
jothan
I had to take the RefCell out, because wait() would await holding &RefCell, making the futures require Sync.
It was replaced by a Mutex, since the socket read/write splitting design is not clear yet.