Skip to content

Prevent background process from crashing test when writing to the test output helper #20254

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

Merged
merged 4 commits into from
Mar 27, 2020

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Mar 27, 2020

I think I know what's causing the crashes.

The interop tests are set up like the following:

A single server is launched in a test fixture for all test cases.
The server is passed an ITestOutputHelper that's used during the entire duration of the tests.
If the test server tries to write anything to the ITestOutputHelper in between the test cases, the ITestOutputHelper is in an invalid state since no test is active. This causes a crash.

I think the proper fix is to have the server not write to the ITestOutputHelper in between test cases. As a tactical fix though I think we can just ignore these exceptions when they are thrown.

Or better yet, use file logging instead so there's no contention. In this case, the server maintains its own logs and write to file. During each test case run, the server also writes to the test case's file logs so that it's easy to correlate between server/client logs.

We ended up deciding to just launch a server instance for each test case.

@Pilchie
Copy link
Member

Pilchie commented Mar 27, 2020

Oops. Accidental click.

@Pilchie Pilchie reopened this Mar 27, 2020
@JunTaoLuo JunTaoLuo added the area-grpc Includes: GRPC wire-up, templates label Mar 27, 2020
@JunTaoLuo
Copy link
Contributor Author

Not working on my dev machine so I can't build while I'm setting up my backup box. Let's see how many attempts it takes me to get it right when coding blind.

@JunTaoLuo JunTaoLuo requested a review from JamesNK March 27, 2020 20:54
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Would be nice to shutdown the server gracefully, but killing the server process should be fine. Can revisit if there is a need in the future.

@JamesNK JamesNK merged commit e79ba29 into master Mar 27, 2020
@JamesNK JamesNK deleted the johluo/debug-interop branch March 27, 2020 23:43
@Tratcher
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-grpc Includes: GRPC wire-up, templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants