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.

Sign up for GitHub

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

Jump to bottom

Precondition that read doesn't return EINVAL #2022

Merged
merged 2 commits into from
Jan 12, 2022
Merged

Precondition that read doesn't return EINVAL #2022

merged 2 commits into from
Jan 12, 2022

Conversation

FranzBusch
Copy link
Member

Motivation:

read should never return EINVAL. If it does return it though it is a bug inside NIO and we should precondition for it similar to what we do for EBADF & EFAULT. Closes #1339

Modifications:

This PR adds a new precondition for EINVAL and adds test that exercise this precondition.

Result:

We are now preconditioning against read returning EINVAL.

@FranzBusch
Copy link
Member Author

@swift-server-bot test this please

@FranzBusch
Copy link
Member Author

@weissi Do you have any idea why these tests are failing? They are also failing for me locally on main and I cannot figure out why they are returning the wrong value now.

@Lukasa Lukasa added the semver/patch No public API change. label Jan 4, 2022
@FranzBusch FranzBusch force-pushed the feature/precondition-einval branch 2 times, most recently from f344c9e to ac54847 Compare January 4, 2022 16:12
Lukasa
Lukasa requested changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

This change is good, but it's a bit too narrow and a bit too broad.

The original issue only asked to forbid EINVAL on read. This change adds it to all syscalls in NIOCore, but does not affect those in NIOPosix, which is where the relevant read is actually made. As a result, it does not resolve the original issue.

Additionally, the change as written affects all system calls. However, some EINVALs we actually want to tolerate. As an example, see #1030. As a result, we likely need a more complex solution that only adds EINVAL policing to some specific system calls, not all of them.

@FranzBusch FranzBusch force-pushed the feature/precondition-einval branch 4 times, most recently from 14ad972 to bfb8b10 Compare January 5, 2022 18:08
@Lukasa
Copy link
Contributor

Lukasa commented Jan 6, 2022

@swift-nio-bot test perf please

@swift-server-bot
Copy link

performance report

build id: 94

timestamp: Sun Dec 19 22:28:51 UTC 2021

results

nameminmaxmeanstd
write_http_headers 0.004327889 0.004367226 0.0043388056000000005 1.2098251693713685e-05
http_headers_canonical_form 0.088313926 0.088894514 0.0885980715 0.0002588511000608681
http_headers_canonical_form_trimming_whitespace 0.167576066 0.168452826 0.1680889416 0.0002595155332337564
http_headers_canonical_form_trimming_whitespace_from_short_string 0.153312184 0.154255371 0.15383771129999999 0.00024360109396810535
http_headers_canonical_form_trimming_whitespace_from_long_string 0.238585713 0.239863572 0.238963447 0.0003816341926988294
bytebuffer_write_12MB_short_string_literals 0.522595906 0.531268526 0.5258848364 0.0026211506984407827
bytebuffer_write_12MB_short_calculated_strings 0.518989762 0.522544016 0.521015271 0.0011868318760418
bytebuffer_write_12MB_medium_string_literals 0.173239381 0.174403759 0.17398923179999998 0.0003783752304342553
bytebuffer_write_12MB_medium_calculated_strings 0.227553147 0.228274979 0.22775705699999998 0.00024319988477381532
bytebuffer_write_12MB_large_calculated_strings 0.143218467 0.146358358 0.14407575239999998 0.0008718310160746891
bytebuffer_lots_of_rw 0.443689603 0.458083832 0.44580180739999997 0.00446523473735646
bytebuffer_write_http_response_ascii_only_as_string 0.039796384 0.040752661 0.0400262595 0.0003163816981190959
bytebuffer_write_http_response_ascii_only_as_staticstring 0.030798481 0.031355696 0.030934602900000004 0.00021863394540365128
bytebuffer_write_http_response_some_nonascii_as_string 0.039851959 0.040395158 0.0399896527 0.000199959591825754
bytebuffer_write_http_response_some_nonascii_as_staticstring 0.03080828 0.031357437 0.0309278487 0.00022576392975261718
no-net_http1_10k_reqs_1_conn 0.108125259 0.109639825 0.10888724949999999 0.0004615764364831673
http1_10k_reqs_1_conn 0.606626788 0.613986334 0.6101183122 0.002359946743328081
http1_10k_reqs_100_conns 0.598891389 0.601216358 0.6001723527 0.0006577159486718654
future_whenallsucceed_100k_immediately_succeeded_off_loop 0.072556074 0.073587663 0.07294410900000001 0.000329831719204069
future_whenallsucceed_100k_immediately_succeeded_on_loop 0.073350804 0.080769139 0.0745249241 0.0022073648264272273
future_whenallsucceed_100k_deferred_off_loop 0.23269406 0.235591944 0.2337350764 0.0009480133427525856
future_whenallsucceed_100k_deferred_on_loop 0.125172072 0.127861923 0.1263387926 0.0009244676353474619
future_whenallcomplete_100k_immediately_succeeded_off_loop 0.031336242 0.032006268 0.0315120554 0.00023447456527412268
future_whenallcomplete_100k_immediately_succeeded_on_loop 0.031048669 0.031618116 0.0312573722 0.00020095603695767145
future_whenallcomplete_100k_deferred_off_loop 0.161715754 0.169714676 0.1651956378 0.002210682755700341
future_whenallcomplete_100k_deferred_on_loop 0.063976463 0.070111544 0.0650170579 0.0018160634089299638
future_reduce_10k_futures 0.037263875 0.037990077 0.0376014212 0.0002197399842256987
future_reduce_into_10k_futures 0.036779503 0.037784655 0.0372948589 0.00028483017036334417
channel_pipeline_1m_events 0.097129569 0.097264339 0.09719843780000001 6.0411770752984136e-05
websocket_encode_50b_space_at_front_1m_frames_cow 0.495406042 0.496200374 0.495761377 0.0002988792606666842
websocket_encode_50b_space_at_front_1m_frames_cow_masking 0.066793175 0.067529005 0.0671142544 0.0002398840434062553
websocket_encode_1kb_space_at_front_100k_frames_cow 0.052287748 0.052770616 0.052439714699999994 0.00021899014143109515
websocket_encode_50b_no_space_at_front_1m_frames_cow 0.495508244 0.496057948 0.4957840979 0.00023676810454603702
websocket_encode_1kb_no_space_at_front_100k_frames_cow 0.052277683 0.052745523 0.052464657299999995 0.00021462774947442263
websocket_encode_50b_space_at_front_10k_frames 0.006586617 0.006615603 0.006598507700000001 8.477318850910361e-06
websocket_encode_50b_space_at_front_10k_frames_masking 0.082228013 0.082847903 0.0825180761 0.00024889037158264045
websocket_encode_1kb_space_at_front_1k_frames 0.000764156 0.000771338 0.0007684919 2.2918072659899806e-06
websocket_encode_50b_no_space_at_front_10k_frames 0.006525779 0.006556422 0.0065334421 8.954392831950665e-06
websocket_encode_1kb_no_space_at_front_1k_frames 0.000697543 0.000703451 0.0007015768 2.228642127694195e-06
websocket_decode_125b_100k_frames 0.115382838 0.116433872 0.1158738341 0.0003420662622298168
websocket_decode_125b_with_a_masking_key_100k_frames 0.121134163 0.122111217 0.12170028829999999 0.0003315248772686771
websocket_decode_64kb_100k_frames 0.121726714 0.122737219 0.1222751737 0.0003224334582411442
websocket_decode_64kb_with_a_masking_key_100k_frames 0.123880376 0.124983958 0.12448534470000001 0.00036749443378250314
websocket_decode_64kb_+1_100k_frames 0.121431134 0.122216976 0.1218172537 0.00023508404632955807
websocket_decode_64kb_+1_with_a_masking_key_100k_frames 0.123940571 0.124765846 0.12431871489999999 0.00022445786863386244
circular_buffer_into_byte_buffer_1kb 0.041229624 0.041683217 0.0413324261 0.00018102215933995592
circular_buffer_into_byte_buffer_1mb 0.082258278 0.08272794 0.082453953 0.00021718901027548262
byte_buffer_view_iterator_1mb 0.020482867 0.020919589 0.0205376709 0.00013465836844602118
byte_to_message_decoder_decode_many_small 0.17503549 0.175601954 0.1755099134 0.00016916402013509958
generate_10k_random_request_keys 0.089743079 0.090840092 0.0901225992 0.0002862033878213178
bytebuffer_rw_10_uint32s 0.307918455 0.30989937 0.3086575716 0.0005684505986123352
bytebuffer_multi_rw_10_uint32s 0.056304739 0.058058611 0.057128081 0.0005948492968385626
lock_1_thread_10M_ops 0.178784859 0.179241473 0.1789488475 0.00012452551881375113
lock_2_threads_10M_ops 0.378932489 0.382517131 0.3804090705 0.0012220596982414804
lock_4_threads_10M_ops 0.373160936 0.380722351 0.3770068465 0.002341348030410226
lock_8_threads_10M_ops 0.354516861 0.363188831 0.3594618964 0.0026757271329534223
schedule_10000_tasks 0.00463565 0.006914705 0.0050068871 0.0006822045999659976
schedule_and_run_10000_tasks 0.018207777 0.018626763 0.0184357393 0.00011809835906099598
execute_10000 0.0088208 0.009199849 0.0088655258 0.00011765579776034064

comparison

name current previous winner diff
write_http_headers 0.004327889 0.004130955 previous 4%
http_headers_canonical_form 0.088313926 0.087564831 previous 0%
http_headers_canonical_form_trimming_whitespace 0.167576066 0.16528233 previous 1%
http_headers_canonical_form_trimming_whitespace_from_short_string 0.153312184 0.15054043 previous 1%
http_headers_canonical_form_trimming_whitespace_from_long_string 0.238585713 0.232052703 previous 2%
bytebuffer_write_12MB_short_string_literals 0.522595906 0.54261672 current -3%
bytebuffer_write_12MB_short_calculated_strings 0.518989762 0.540313895 current -3%
bytebuffer_write_12MB_medium_string_literals 0.173239381 0.180992313 current -4%
bytebuffer_write_12MB_medium_calculated_strings 0.227553147 0.22829601 current 0%
bytebuffer_write_12MB_large_calculated_strings 0.143218467 0.146980356 current -2%
bytebuffer_lots_of_rw 0.443689603 0.457027357 current -2%
bytebuffer_write_http_response_ascii_only_as_string 0.039796384 0.041238087 current -3%
bytebuffer_write_http_response_ascii_only_as_staticstring 0.030798481 0.032513868 current -5%
bytebuffer_write_http_response_some_nonascii_as_string 0.039851959 0.041219108 current -3%
bytebuffer_write_http_response_some_nonascii_as_staticstring 0.03080828 0.032839136 current -6%
no-net_http1_10k_reqs_1_conn 0.108125259 0.105981338 previous 2%
http1_10k_reqs_1_conn 0.606626788 0.610585318 current 0%
http1_10k_reqs_100_conns 0.598891389 0.600179965 current 0%
future_whenallsucceed_100k_immediately_succeeded_off_loop 0.072556074 0.073975996 current -1%
future_whenallsucceed_100k_immediately_succeeded_on_loop 0.073350804 0.076267747 current -3%
future_whenallsucceed_100k_deferred_off_loop 0.23269406 0.235500652 current -1%
future_whenallsucceed_100k_deferred_on_loop 0.125172072 0.127817847 current -2%
future_whenallcomplete_100k_immediately_succeeded_off_loop 0.031336242 0.031537359 current 0%
future_whenallcomplete_100k_immediately_succeeded_on_loop 0.031048669 0.031995454 current -2%
future_whenallcomplete_100k_deferred_off_loop 0.161715754 0.163252267 current 0%
future_whenallcomplete_100k_deferred_on_loop 0.063976463 0.063992548 current 0%
future_reduce_10k_futures 0.037263875 0.038744145 current -3%
future_reduce_into_10k_futures 0.036779503 0.037939393 current -3%
channel_pipeline_1m_events 0.097129569 0.097133848 current 0%
websocket_encode_50b_space_at_front_1m_frames_cow 0.495406042 0.50354132 current -1%
websocket_encode_50b_space_at_front_1m_frames_cow_masking 0.066793175 0.068276485 current -2%
websocket_encode_1kb_space_at_front_100k_frames_cow 0.052287748 0.053521414 current -2%
websocket_encode_50b_no_space_at_front_1m_frames_cow 0.495508244 0.504286439 current -1%
websocket_encode_1kb_no_space_at_front_100k_frames_cow 0.052277683 0.053388175 current -2%
websocket_encode_50b_space_at_front_10k_frames 0.006586617 0.00687367 current -4%
websocket_encode_50b_space_at_front_10k_frames_masking 0.082228013 0.085340452 current -3%
websocket_encode_1kb_space_at_front_1k_frames 0.000764156 0.000794229 current -3%
websocket_encode_50b_no_space_at_front_10k_frames 0.006525779 0.006833091 current -4%
websocket_encode_1kb_no_space_at_front_1k_frames 0.000697543 0.000722303 current -3%
websocket_decode_125b_100k_frames 0.115382838 0.119325792 current -3%
websocket_decode_125b_with_a_masking_key_100k_frames 0.121134163 0.122654004 current -1%
websocket_decode_64kb_100k_frames 0.121726714 0.122116801 current 0%
websocket_decode_64kb_with_a_masking_key_100k_frames 0.123880376 0.125497145 current -1%
websocket_decode_64kb_+1_100k_frames 0.121431134 0.122298403 current 0%
websocket_decode_64kb_+1_with_a_masking_key_100k_frames 0.123940571 0.125586031 current -1%
circular_buffer_into_byte_buffer_1kb 0.041229624 0.041262974 current 0%
circular_buffer_into_byte_buffer_1mb 0.082258278 0.082256567 previous 0%
byte_buffer_view_iterator_1mb 0.020482867 0.020487196 current 0%
byte_to_message_decoder_decode_many_small 0.17503549 0.175666607 current 0%
generate_10k_random_request_keys 0.089743079 0.089279399 previous 0%
bytebuffer_rw_10_uint32s 0.307918455 0.305610109 previous 0%
bytebuffer_multi_rw_10_uint32s 0.056304739 0.056318408 current 0%
lock_1_thread_10M_ops 0.178784859 0.159197135 previous 12%
lock_2_threads_10M_ops 0.378932489 0.79521017 current -52%
lock_4_threads_10M_ops 0.373160936 0.870138925 current -57%
lock_8_threads_10M_ops 0.354516861 1.065500287 current -66%
schedule_10000_tasks 0.00463565 0.004546345 previous 1%
schedule_and_run_10000_tasks 0.018207777 0.018137167 previous 0%
execute_10000 0.0088208 0.008801241 previous 0%

significant differences found

@Lukasa
Copy link
Contributor

Lukasa commented Jan 6, 2022

Yup, that report above is not right: those are clearly the results from @weissi's previous PR.

@Lukasa
Copy link
Contributor

Lukasa commented Jan 6, 2022

@tomerd Seems like the NIO performance builds aren't working again.

@FranzBusch FranzBusch requested a review from Lukasa January 7, 2022 10:24
@FranzBusch
Precondition that read doesn't return EINVAL
869fca6 
### Motivation:

`read` should never return `EINVAL`. If it does return it though it is a bug inside NIO and we should precondition for it similar to what we do for `EBADF` & `EFAULT`. Closes apple#1339

### Modifications:

This PR adds a new precondition for `EINVAL` and adds test that exercise this precondition.

### Result:

We are now preconditioning against `read` returning `EINVAL`.
@FranzBusch FranzBusch enabled auto-merge (squash) January 11, 2022 12:22
Lukasa
Lukasa approved these changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM, nice one!

@FranzBusch FranzBusch merged commit 51c3fc2 into apple:main Jan 12, 2022
@FranzBusch FranzBusch deleted the feature/precondition-einval branch January 12, 2022 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weissi weissi weissi left review comments

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

we should assert/precondition that read doesn't return EINVAL
4 participants
@FranzBusch @Lukasa @swift-server-bot @weissi