Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Don't Flush read-only streams from CryptoStream - #27326 #27327

Closed
wants to merge 1 commit into from

Conversation

kekekeks
Copy link

@kekekeks kekekeks commented Feb 21, 2018

@stephentoub
Copy link
Member

cc: @bartonjs

@@ -128,7 +128,7 @@ public void FlushFinalBlock()
innerCryptoStream.FlushFinalBlock();
}
}
else
else if (_canWrite)
Copy link
Member

Choose a reason for hiding this comment

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

A FileStream opened in read mode still has semantic behaviors on Flush, and not calling it could have observable consequences to caller code (and that was simply the first kind of stream I looked at). I don't know that there's a safe way to make this change.

Copy link
Author

Choose a reason for hiding this comment

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

So, basically, you are telling that my issue (CryptoStream throwing exceptions in Dispose) is a bug in Kestrel server that doesn't allow to call Flush on read-only stream, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sorry. If they can't/won't fix it you could add yet another Stream as a wrapper over it, which just intercepts the call to Flush and doesn't pass it on.

I checked the .NET Framework 2.0 source base, and CryptoStream definitely had this behavior back that far.

kekekeks added a commit to kekekeks/KestrelHttpServer that referenced this pull request Feb 21, 2018
Because read-only streams apparently can have Flush semantics and this behavior is expected by some of built-in stream wrappers (e. g. CryptoStream)

dotnet/corefx#27327 (review)
aspnet#2341
@stephentoub
Copy link
Member

@kekekeks, thanks for your interest in contributing.

@bartonjs, it sounds like you think this should be closed?

@bartonjs
Copy link
Member

it sounds like you think this should be closed?

Yeah, it's a breaking change; so it would require exposing options... or just fixing the stream type which is throwing on Flush (which is the path being pursued).

@bartonjs bartonjs closed this Feb 22, 2018
kekekeks added a commit to kekekeks/KestrelHttpServer that referenced this pull request Feb 23, 2018
Because read-only streams apparently can have Flush semantics and this behavior is expected by some of built-in stream wrappers (e. g. CryptoStream)

dotnet/corefx#27327 (review)
aspnet#2341
kekekeks added a commit to kekekeks/KestrelHttpServer that referenced this pull request Feb 23, 2018
Because read-only streams apparently can have Flush semantics and this behavior is expected by some of built-in stream wrappers (e. g. CryptoStream)

dotnet/corefx#27327 (review)
aspnet#2341
kekekeks added a commit to kekekeks/KestrelHttpServer that referenced this pull request Feb 23, 2018
Because read-only streams apparently can have Flush semantics and this behavior is expected by some of built-in stream wrappers (e. g. CryptoStream)

dotnet/corefx#27327 (review)
aspnet#2341
davidfowl pushed a commit to aspnet/KestrelHttpServer that referenced this pull request Feb 23, 2018
Because read-only streams apparently can have Flush semantics and this behavior is expected by some of built-in stream wrappers (e. g. CryptoStream)

dotnet/corefx#27327 (review)
#2341
@karelz karelz added this to the 2.1.0 milestone Mar 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants