fix: WiFiClient::flush() yields but can be called from events #5254
Conversation
| yield(); | ||
| // yield because we are staying longer | ||
| // (only if possible because we can be called from events) | ||
| esp_yield(); |
There was a problem hiding this comment.
Did you mean to move the esp_yield inside the if-block? Before it would yield each loop, here it will only yield if it actually sends something so it seems like it'll hard-lock, no?
There was a problem hiding this comment.
In doubt, you are right.
We are waiting for acks to give us the right to send, and they are supposed to come asynchronously. So yielding here is only for avoiding wdt.
There was a problem hiding this comment.
The esp_yield() should not be conditional.
Also, while in the sys context, i.e.: in an async callback: if we're relying on receiving something and detecting that reception during the yield, that won't happen => infinite loop. The async way of doing this is to break the loop body into its own function, and schedule the function to get called by the sys context. Then, in the function body, if nothing has happened and we need to continue waiting, the function is re-scheduled, and return.
Personally, I don't like the idea of trying to allow sync calls during async callbacks. In fact, I would disallow such usage, period. The call sequence and use models are very different, and there's bound to be something that will bite hard. Async callbacks in the sys context have stricter timing requirements (e.g.: sending several packets and waiting for the acks or whatever could go boom), you can't yield and then continue execution, if you're waiting on something you have to return and come back later, etc.
What I would love to have is an async socket that can work from async callbacks, and a sync socket that is built on top of it by serializing (i.e.: waiting for) the async calls. One possibility would be to implement a new WiFiClient/Server built on top of @me-no-dev 's async libs. I'm not sure how that serialization would be implemented, I'd have to think about it, but if we did manage it, we could drop a lot of code, unify with the async libs, get an async framework into the core, and overall get lots of goodies.
| yield(); | ||
| // yield because we are staying longer | ||
| // (only if possible because we can be called from events) | ||
| esp_yield(); |
There was a problem hiding this comment.
The esp_yield() should not be conditional.
Also, while in the sys context, i.e.: in an async callback: if we're relying on receiving something and detecting that reception during the yield, that won't happen => infinite loop. The async way of doing this is to break the loop body into its own function, and schedule the function to get called by the sys context. Then, in the function body, if nothing has happened and we need to continue waiting, the function is re-scheduled, and return.
Personally, I don't like the idea of trying to allow sync calls during async callbacks. In fact, I would disallow such usage, period. The call sequence and use models are very different, and there's bound to be something that will bite hard. Async callbacks in the sys context have stricter timing requirements (e.g.: sending several packets and waiting for the acks or whatever could go boom), you can't yield and then continue execution, if you're waiting on something you have to return and come back later, etc.
What I would love to have is an async socket that can work from async callbacks, and a sync socket that is built on top of it by serializing (i.e.: waiting for) the async calls. One possibility would be to implement a new WiFiClient/Server built on top of @me-no-dev 's async libs. I'm not sure how that serialization would be implemented, I'd have to think about it, but if we did manage it, we could drop a lot of code, unify with the async libs, get an async framework into the core, and overall get lots of goodies.
|
Hi @d-a-v, @earlephilhower, @devyte We have done several tests and we found that this last commit make the device unresponsive to webserver. Reverting to ad7cb63 all works again. Please, could you revert this last commit? |
|
Hi, As discussed in e549355 changing Thanks. 👍 |
Replace esp_yield with delay(0) which calls both the yield and the scheduler. Supersedes PR esp8266#5254
for #5237
Bug introduced by #5167 which replaced
delay()byyield(),and that should have been
esp_yield()which is the onedelay()calls.To be noted for current maintainers included myself:
delay()andesp_yield()can be called from sys context