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

Fixing intermittent failures due to js-quic stream errors #725

Merged
merged 3 commits into from
May 23, 2024

Conversation

tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented May 23, 2024

Description

This PR addressed #722

The main problem was during certain circumstances an error that should've been thrown through a stream was escaping the call stack and crashing the program. This was due to improper usage of throw within the stream handlers. This has been resolved within js-quic.

Note that the branch name doesn't describe the problem scope very well. It was created while I was still exploring what the actual issue was. While mainnet connections in tests could still happen, they shouldn't be causing this issue regardless. No amount of interacting with random network traffic should crash the program.

#461 was removed from this PR, it turns out to not actually be a requirement.

Issues Fixed

Tasks

  • 1. find the problem causing intermittent errors.
  • 2. Apply the fix to js-quic and update the dependency version pulling in the fix
  • 3. Applied any other minor fixes

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@tegefaulkes tegefaulkes self-assigned this May 23, 2024
Copy link

linear bot commented May 23, 2024

@tegefaulkes tegefaulkes force-pushed the feature-network-test-isolation branch from 378e96a to d79de26 Compare May 23, 2024 05:06
@tegefaulkes tegefaulkes requested a review from amydevs May 23, 2024 05:06
@tegefaulkes tegefaulkes merged commit 6c8385e into staging May 23, 2024
@CMCDragonkai
Copy link
Member

What are the implications of doing this:

Given that throw reason; and related were supposed to be "proper".

@tegefaulkes
Copy link
Contributor Author

Everything functions the same way, we just remove the chance of that throw inside the stream handler bubbling up and crashing. It's not a good idea to throw the errors inside the stream like that, I think I pointed it out at the time as well. So I've removed the throw in the cases where the errors are only expected to be thrown through the stream.

@CMCDragonkai
Copy link
Member

If it still happens on CI, are you sure the fixes actually fix this? If not, perhaps it's better to leave it as is, and find the actual deterministic root cause.

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

Successfully merging this pull request may close these issues.

Fix random test failures caused by mainnet connections
2 participants