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

webtransport: return error before wrapping opened / accepted streams #1775

Merged
merged 1 commit into from
Sep 22, 2022
Merged

webtransport: return error before wrapping opened / accepted streams #1775

merged 1 commit into from
Sep 22, 2022

Conversation

marten-seemann
Copy link
Contributor

No description provided.

Jorropo
Jorropo suggested changes Sep 22, 2022
View reviewed changes
p2p/transport/webtransport/conn.go Outdated Show resolved Hide resolved
p2p/transport/webtransport/conn.go Outdated Show resolved Hide resolved
Jorropo
Jorropo approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@Jorropo Jorropo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Except what I sent before (details but should be fixed for readability), SGTM
I don't have the big picture context tho.

@Jorropo
Copy link
Contributor

Jorropo commented Sep 22, 2022 •
edited
Loading

What I don't get is that there must be broken code elsewhere.
Seems like someone is ignoring err and or checking if the stream is nil instead.

@marten-seemann
Copy link
Contributor Author

What I don't get is that there must be broken code elsewhere.
Seems like someone is ignoring err and or checking if the stream is nil instead.

That's if the bug is upstream. If the bug is downstream, we might get a nil, nil return, and this fix won't work (it's still the right thing to do).

@marten-seemann marten-seemann merged commit 031ec50 into master Sep 22, 2022
@Jorropo Jorropo deleted the webtransport-fix-stream-panic branch September 22, 2022 08:10
@marten-seemann marten-seemann mentioned this pull request Sep 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jorropo Jorropo Jorropo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marten-seemann @Jorropo