Skip to content

gnrc_sock: re-issue MSG_RECV event if there are still received messages after *_recv() was called.#14059

Merged
kb2ma merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_sock/fix/re-recv-signal
May 15, 2020
Merged

gnrc_sock: re-issue MSG_RECV event if there are still received messages after *_recv() was called.#14059
kb2ma merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_sock/fix/re-recv-signal

Conversation

@miri64
Copy link
Member

@miri64 miri64 commented May 12, 2020 •
edited
Loading

Contribution description

This prevents a client from needing to call *_recv() in a loop.

Testing procedure

See #14034

Issues/PRs references

Fixes #14034

@miri64
gnrc_sock: re-issue MSG_RECV event if there are still received messages
d66583b 
... after `*_recv()` was called.

This prevents a client from needing to call `*_recv()` in a loop.
@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch CI: run tests If set, CI server will run tests on hardware for the labeled PR labels May 12, 2020
@miri64 miri64 mentioned this pull request May 12, 2020
Closed
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 12, 2020
@haukepetersen
Copy link
Contributor

haukepetersen commented May 13, 2020

I might have seen this issue in my CoAP-over-BLE experiements lately, at least the results looked quite bad despite some other bugfixes that were done lately (notably #13962 and #13850). So there might be a chance that the issue described in #14034 has something to do with it.

I just started my experiment with this patch included - will let you know in 12h if something has changed :-)

In the mean time, it would be nice if anyone could take a look at #13962, otherwise it is getting cumbersome to keep track of all the fixes I need...

@kb2ma
Copy link
Member

kb2ma commented May 13, 2020

Thanks for the information, @haukepetersen. I am actively reviewing, and should have some feedback in the next day or so.

@kb2ma
Copy link
Member

kb2ma commented May 15, 2020

Works for me. I have a question though: Why is this fix necessary? Why doesn't the normal mechanism call _netapi_cb() to post the event?

@miri64
Copy link
Member Author

miri64 commented May 15, 2020

Works for me. I have a question though: Why is this fix necessary? Why doesn't the normal mechanism call _netapi_cb() to post the event?

Because every sock has just one sock_event_t to post. So if the event is not drained from the queue before the next post, it will just be re-added to the queue. @kaspar030 and I actually planned it to implement it like I do in the fix now. It just was forgotten.

@miri64
Copy link
Member Author

miri64 commented May 15, 2020

The alternative would be to have a pool of sock_event_t objects to allocate. This however, would not be very memory friendly and could lead to starvation.

@kb2ma
Copy link
Member

kb2ma commented May 15, 2020

Because every sock has just one sock_event_t to post. So if the event is not drained from the queue before the next post, it will just be re-added to the queue.

So at the end of gnrc_sock_recv() we can assume the user is pulling the latest event. This is the right time to check for more incoming data and post an event if present. Makes sense to me now.

@kb2ma
Copy link
Member

kb2ma commented May 15, 2020 •
edited
Loading

Looks like a spurious failure on last Murdock run. Let's try again.

@kb2ma kb2ma added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 15, 2020
@miri64
Copy link
Member Author

miri64 commented May 15, 2020

Murdock is happy now :-).

@miri64 miri64 mentioned this pull request May 15, 2020
Merged
@kb2ma
Copy link
Member

kb2ma commented May 15, 2020

Perfect. Let's go!

@kb2ma kb2ma merged commit f81dda2 into RIOT-OS:master May 15, 2020
@miri64 miri64 deleted the gnrc_sock/fix/re-recv-signal branch May 15, 2020 15:42
@miri64
Copy link
Member Author

miri64 commented May 15, 2020

Don't forget its brother #14060.

@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
@miri64 miri64 modified the milestone: Release 2020.07 Jul 23, 2020
@miri64 miri64 removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kb2ma kb2ma kb2ma approved these changes

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2020.07

Development

Successfully merging this pull request may close these issues.

gcoap server breaks or becomes very unresponsive (sock_async related?)

4 participants

@miri64 @haukepetersen @kb2ma @benpicco