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

Implement IStream::Seek on IRequestStream to accommodate reuse #1573

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mschofie
Copy link
Member

This PR adds an IStream implementation to http_client_winrt.cpp's IRequestStream in order to support IStream::Seek and so stream reuse. This addresses #1572.

An implementation of ISequentialStream is passed into IXMLHttpRequest2::Send to represent the body of an HTTP request. If IXMLHttpRequest2 hits a recoverable error - in my case ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED when POST'ing to a server that accepts but doesn't require a client certificate - it will attempt to retry the send. In order to do so IXMLHttpRequest2 needs to seek the stream back to the start (having already read the buffer once for the original request), but ISequentialStream doesn't support seeking. If the ISequentialStream can be QI'd for IStream, then IXMLHttpRequest2 will use that to seek back to the start, and the request will be retried.

The implementation of IStream is very basic, everything but Seek returns E_NOTIMPL. The Seek implementation itself is relatively straight-forward, since the constructs map directly through to the underlying buffer.

@barcharcraz
Copy link
Member

It seems odd to me to change the implementation to IStream and then sometimes say "sorry I'm not actually an IStream". In particular it seems to me that any user of this new IRequestStream that actually relies on most of the invariants of the IStream interface is going to be in for a nasty suprise

@mschofie
Copy link
Member Author

I don't think it's sometimes saying "sorry I'm not actually an IStream", it's implementing the IStream interface with limited functionality. I believe this is consistent with the current IRequestStream implementation returning E_NOTIMPL for its Write method, or IResponseStream returning E_NOTIMPL for its Read method. The construct of a seekable, read-only IStream seems useful and I'm not sure how else it would be implemented. If you think there's value in adding more to the IStream implementation, I can take a look at that.

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.

2 participants