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

Shutdown Stream on Send/Start Failure #3637

Merged
merged 6 commits into from
May 19, 2023
Merged

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented May 17, 2023

Description

Fixes #3638 and #3639. In the rare case (OOM) that stream start fails when you use the QUIC_SEND_FLAG_START flag, it should be delivering a shutdown complete event to indicate this failure to start. Also fixes more event issues in the case of OOM in the send flush operation alloc path.

Testing

Existing automation and manual testing from original bug opener.

Documentation

N/A

@nibanks nibanks requested a review from a team as a code owner May 17, 2023 17:33
@nibanks nibanks added the Bug: Core A code bug in the Core MsQuic code label May 17, 2023
src/core/api.c Outdated Show resolved Hide resolved
csujedihy
csujedihy previously approved these changes May 17, 2023
@csujedihy
Copy link
Contributor

there's a build error. Finding the error in the github action is like finding a needle in a haystack. The console coloring is all gone.

/home/runner/work/msquic/msquic/src/core/connection.c:1873:9: error: too few arguments to function ‘QuicConnShutdown’
 1873 |         QuicConnShutdown(Connection, ShutdownFlags, ShutdownErrorCode, FALSE);
      |         ^~~~~~~~~~~~~~~~
/home/runner/work/msquic/msquic/src/core/connection.c:437:1: note: declared here
  437 | QuicConnShutdown(
      | ^~~~~~~~~~~~~~~~

@nibanks
Copy link
Member Author

nibanks commented May 18, 2023

Feel free to push a fix, otherwise I will tomorrow.

Copy link

Choose a reason for hiding this comment

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

May need to add 3 unit test cases to cover the following:

  • If StreamSend returns SUCCESS, SEND_COMPLETE must be indicated exactly-once
  • If StreamSend returns FAIL, SEND_COMPLETE must not be indicated.
  • If StreamSend(QUIC_SEND_FLAG_START) returns SUCCESS, STREAM_SHUTDOWN_COMPLETE must be indicated exactly-once.

@nibanks nibanks merged commit ce6e932 into main May 19, 2023
@nibanks nibanks deleted the nibanks/stream-shutdown-on-failure branch May 19, 2023 16:47
nibanks added a commit that referenced this pull request May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Core A code bug in the Core MsQuic code
Projects
None yet
3 participants