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

Move datagrams below regular streams #1447

Closed
wants to merge 5 commits into from

Conversation

martinthomson
Copy link
Member

This will starve datagrams, which comes with its own risks.

Eventually, we'll need a better strategy.

@jesup is this a better starting for #1440?

This will starve datagrams, which comes with its own risks.

Eventually, we'll need a better strategy.
@jesup
Copy link
Member

jesup commented Jul 2, 2023

LGTM. Both ways work.

@KershawChang
Copy link
Collaborator

Hi @martinthomson , I am a bit confused about this patch.
Should we land this one or #1440? @jesup thinks we should land this one, but this is still a draft.
What do you think?
Thanks.

@KershawChang
Copy link
Collaborator

@martinthomson , could you resolve the conflicts of this patch?
I'd like to land this one, since this also includes some clean up. Thanks.

@martinthomson
Copy link
Member Author

The blocker here is tests. Neither patch includes tests and I really don't like having functionality that isn't tested.

@KershawChang
Copy link
Collaborator

The blocker here is tests. Neither patch includes tests and I really don't like having functionality that isn't tested.

Ah, I see. I'll write a test for this.

@KershawChang KershawChang mentioned this pull request Jul 7, 2023
@martinthomson
Copy link
Member Author

I think that we got the tests in #1440, so I'm closing this one.

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.

3 participants