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

Limit size of equeue for sigio netsocket test #5474

Merged
merged 1 commit into from
Nov 13, 2017

Conversation

sarahmarshy
Copy link
Contributor

Description

This fixes netsocket failure on arch pro target by limiting the size of EventQueue instance.

Status

READY

Related to #5335

@geky

Copy link
Contributor

@geky geky left a comment

Choose a reason for hiding this comment

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

💯

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 10, 2017

For future, add a reason why we are setting this to 4 (previous number to current one). how was 4 selected? Any magic number has a reason, and could be stated in the code or rather in the commit message.

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 10, 2017

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 10, 2017

Build : SUCCESS

Build number : 484
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5474/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Nov 10, 2017

@mbed-ci
Copy link

mbed-ci commented Nov 10, 2017

@kjbracey
Copy link
Contributor

Stack overflow problem, was it?

This code looks a bit scary to me - you're declaring threads and event queues on the stack. I'm kind of surprised that it shuts down the thread cleanly on exit from the test function. Forcible thread termination is a bit scary.

Might I suggest having a static initialised-once event queue+thread? Which would be conveniently available thus:

EventQueue &queue = *mbed_event_queue();

That's an on-demand factory, so it would launch the thread and event queue on first call, and it would remain running between tests.

@sarahmarshy
Copy link
Contributor Author

@kjbracey-arm, maybe that can come as a later PR. I'd like to get this merged ASAP, because #5335 needs to get through to allow network testing to resume.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 13, 2017

@kjbracey-arm, maybe that can come as a later PR. I'd like to get this merged ASAP, because #5335 needs to get through to allow network testing to resume.

@kjbracey-arm Are you happy with this proposal? It will be reworked to as you suggested in the separate PR.

@kjbracey-arm, maybe that can come as a later PR. I'd like to get this merged ASAP, because #5335 needs to get through to allow network testing to resume.

@sarahmarshy Emphasize this type of requirement please, we should be aware of this dependency plus priority definition (the above comment only declares this as related to), will help to get priorities correctly.

@geky
Copy link
Contributor

geky commented Nov 13, 2017

@sarahmarshy Emphasize this type of requirement please, we should be aware of this dependency plus priority definition (the above comment only declares this as related to), will help to get priorities correctly.

@0xc0170, what sort of emphasis are you looking for? We don't really have a system for naming priorities.

@pan-
Copy link
Member

pan- commented Nov 13, 2017

@kjbracey-arm Beside issues which may arise because there is no stack unwinding when a thread is abruptly terminated; the current code may operate on already released memory.

At the end of the scope, the event queue will be destroyed then the eventThread will be terminated; the kernel may schedule the eventThread between those two operations (this depends on the implementation details of the socket object).

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 14, 2017

@0xc0170, what sort of emphasis are you looking for? We don't really have a system for naming priorities.

@geky We shall clarify dependencies (this patch is required for another one to progress, not just related). I failed to see the connection for the other PR. It could be just me but after reading the requirement on another PR comment, this became a clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants