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

Windows: Fix active_quic_listener_test #13918

Closed
wants to merge 1 commit into from
Closed

Windows: Fix active_quic_listener_test #13918

wants to merge 1 commit into from

Conversation

sunjayBhatia
Copy link
Member

Commit Message:
Windows: Fix active_quic_listener_test

Rerun event loop until all client connections are consumed by the event
loop. Running a non-blocking event loop, all outstanding socket events
(especially on Windows) may not be picked up before the dispatcher
exits.

Additional Description:
Another alternative would be to run the run the dispatcher in blocking
mode and make it exit in a mocked callback on the last request
processing. This is doable for the non-TLS quic path, but not as
straightforward when testing a quic version with TLS with the network
filter chain setup etc.

Risk Level: Low
Testing: Locally on Windows + RBE, modifies unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Rerun event loop until all client connections are consumed by the event
loop. Running a non-blocking event loop, all outstanding socket events
(especially on Windows) may not be picked up before the dispatcher
exits.

Another alternative would be to run the run the dispatcher in blocking
mode and make it exit in a mocked callback on the last request
processing. This is doable for the non-TLS quic path, but not as
straightforward when testing a quic version with TLS with the network
filter chain setup etc.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia
Copy link
Member Author

@envoyproxy/windows-dev

@davinci26
Copy link
Member

davinci26 commented Nov 5, 2020

I think this one passes as it is with #13787 so we can either take this PR to fix or I will enable it when #13787 gets merged

@sunjayBhatia
Copy link
Member Author

I think this one passes as it is with #13787 so we can either take this PR to fix or I will disable it when #13787 gets merged

ah interesting, let me rerun the test a few times in RBE on your branch

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Nov 5, 2020

I think this one passes as it is with #13787 so we can either take this PR to fix or I will disable it when #13787 gets merged

ah interesting, let me rerun the test a few times in RBE on your branch

I remember now! we tested this earlier on before your PR was further along and saw it still failing (it is passing now, confirmed), sounds good, will hold off on these sorts of flaky/failing tests for a bit until your PR is reviewed and merged

@sunjayBhatia sunjayBhatia deleted the windows-fix-active-quic-listener-test branch November 5, 2020 17:50
@davinci26
Copy link
Member

I ended up removing the EVLOOP_ONCE flag from libevent which made emulated edge events to behave really similar to edge events. I also enabled the two other event tests that were marked as fails_on_windows without any (significant) changes on windows

@sunjayBhatia
Copy link
Member Author

@davinci26 the general idea around this kind of change still maybe stands, technically non-blocking mode doesn't guarantee every event is picked up, the assumption that it does maybe should be reviewed at some point and test structure at large refactored, especially if we continue to see windows flakiness

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

Successfully merging this pull request may close these issues.

2 participants