Modern HTTP(s) Support by TcT2k · Pull Request #977 · wxWidgets/wxWidgets

TcT2k · 2018-10-14T20:34:30Z

This is my proposal for a simple HTTP request class which would allow modern HTTP and HTTP/s connections based on APIs provided by the operating system.

Please have a look at the interface file to see the complete documentation.

Usage would look like this:

// Create the request object
wxObjectDataPtr<wxWebRequest> request(
    wxWebSession::GetDefault().CreateRequest("https://www.wxwidgets.org/downloads/logos/blocks.png"));
// Bind events
request->Bind(wxEVT_WEBREQUEST_STATE, [](wxWebRequestEvent& evt) {
    switch (evt.GetState())
    {
        // Request completed
        case wxWebRequest::State_Completed:
        {
            wxImage logoImage(*evt->GetResponse()->GetStream());
            if (logoImage.IsOK())
                wxLogInfo("Image loaded");
            break;
        }
        // Request failed
        case wxWebRequest::State_Failed:
            wxLogError("Could not load logo: %s", evt.GetErrorDescription());
            break;
    }
});
// Start the request
request->Start();

As always any feedback is welcome. I don't have an ETA on when or if I would implement this, but I've experience with the proposed underlying APIs and it should be easy to get started.

Initial todo for a basic (but functional) implementation:

Further improvements in the future could be:

More detailed connection options (timeouts, TLS versions, insecure HTTPs, etc)
Use Vista+ CredUIPromptForWindowsCredentialsW for wxCredentialEntryDialog

The following APIs would be used:

Operating System	API	HTTPS	HTTP/2
Windows	WinHTTP	Yes	Windows 10 1607
macOS	NSURLSession	macOS 10.9	macOS 10.11
iOS	NSURLSession	iOS 7.0	iOS 9.0
Linux	libcurl	Yes	7.47.0

Sample App Screenshots

oneeyeman1 · 2018-10-14T20:50:16Z

@TcT2k ,
NSURLSession is available since 10.9. The minimum OSX version for wxWidgets is 10.7.
Is there a fallback for 10.7/10.8?

Also, which *nix libcurl isnot available? Embedded platform? Will this be turned off there? Can configure distinguish such cases?

eranif · 2018-10-14T21:13:13Z

Since most of the applications using this suggested API will be a wxWidgets application, doesn't it make more sense to use events?
For example, Send() can fire events when the operation completes, maybe wxEVT_HTTP_DATA_READY and wxEVT_HTTP_ERROR

Also, in order to make this class usable in a secondary thread, a "sync" API should be offered too. (Maybe it can be based on a style bit: wxHTTP_USE_EVENTS?)
When Send() is used in its event mode, it always return true.

A side note:
The name 'Send' implies that we are "sending", while we are actually loading data from the web. So maybe changing it to something like 'Fetch' or 'Load'?

These are my thoughts :)

vadz

This looks like an excellent start, thanks a lot!

I can think of many possible extensions, of course, but mostly I think they can be safely postponed until (much) later. One of the things I think we do need to decide from the beginning is whether we need to provide support for any native async APIs because it seems clear to me that after implementing this the next step is to provide some wxHTTPRequestThread that would send and receive HTTP requests and communicate with the main thread via events because the main thread should never perform any network operations directly. We will almost certainly need to have a generic version of this anyhow because I don't think libcurl provides any async APIs (or does it?), but if there is native async support anywhere, we need to ensure that the API is compatible with it. Notably, it should be possible to cancel a request.

To answer @oneeyeman1, we will have wxUSE_HTTPREQUEST, of course, and configure will set it to 0 if libcurl is not found, but this is not the interesting part.

Thanks again Tobias!

TcT2k · 2018-10-14T22:02:03Z

@eranif

For example, Send() can fire events when the operation completes, maybe wxEVT_HTTP_DATA_READY and wxEVT_HTTP_ERROR

Sounds like a good idea. To support both usage scenarios it would probably make sense to make two methods like SendAsync and Send.

A side note:
The name 'Send' implies that we are "sending", while we are actually loading data from the web. So maybe changing it to something like 'Fetch' or 'Load'?

Technically we are always sending something (request headers and the URL) and send is also used by something like the XMLHttpRequest in javascript

vadz

Nothing really important, API still looks good to me.

It would be nice to have an example of async use in the class description too, I think it could be made quite short using Bind().

Thanks!

TcT2k · 2018-10-16T21:28:54Z

I've changed to API to async only and changed the structure a little.
Async seems more appropriate and will probably discourage users not to do network request in the main thread.

wxWebRequest are now created by wxWebRequestSession::CreateSession() as a reference counted object instead of their own constructor.

catalinr

Fixed a few typos, and a couple of other comments.

catalinr

A couple more typos

catalinr

and 2 more typos

mtangoo · 2018-10-21T18:28:22Z

Not really to add any invaluable comment but I would lie to thank @TcT2k and everyone taking this. I would love to test it when the need be (in Windows, Mac and Linux)

Great work!

TcT2k · 2018-10-31T13:12:11Z

I've finalized the API to what I think should work with every purposed backend offered by Windows, macOS and libcurl.
The API is completely async and State change events will always be send in the main thread to allow for easy UI updates from them.

The WinHTTP backend is near feature complete. NSURLSession and CURL are just stubs for now, but I've already modeled some of the API after features offered by NSURLSession.

Like the current WinHTTP backend, the NSURLSession backend will not require an extra thread as the underlying API is async. The CURL backend will probably require one thread per session handling the curl_multi_... and select() calls.

MaartenBent · 2018-10-31T19:08:36Z

How will you fix the mingw build? Disable it via configure/cmake when the header/lib is not available, or link dynamic to winhttp?

TcT2k · 2018-10-31T21:34:35Z

How will you fix the mingw build? Disable it via configure/cmake when the header/lib is not available, or link dynamic to winhttp?

Disabling is probably the most sensible when building with incomplete headers, like with other semi "modern" windows apis used by wxWidgets.

No real changes, just use the same name as in the other backends for consistency (we could also rename m_sessionImpl in the other ones to m_sessionCURL and m_sessionURLSession respectively, but this would have been more work and the latter name is really not great).

It's better not to have this method in the public class, even if it means that we need to pass a wxWebSessionImpl object to wxWebRequestImpl ctor explicitly now. No real changes.

This allows to retrieve the handles used internally in order to do something not supported by the public API yet.

vadz · 2021-01-15T23:27:27Z

I don't understand what's going on with the tests under AppVeyor... all we get is

Test program for wxWidgets non-GUI features
build: 3.1.5 (wchar_t,Visual C++ 1500,wx containers,compatible with 3.0)
running under Windows Server 2012 R2 (build 9600), 64-bit edition as appveyor, locale is C
!!! Non-GUI test failed.
Command exited with code -1073741819

i.e. it crashes somewhere without any other information. I can't reproduce this locally, I'll try to find some way to get a stack trace from AppVeyor but if I don't manage to do it quickly, I'll just disable the tests there as I really want to merge this, finally, instead of postponing it yet again.

TcT2k · 2021-01-15T23:33:18Z

I don't understand what's going on with the tests under AppVeyor... all we get is
Test program for wxWidgets non-GUI features
build: 3.1.5 (wchar_t,Visual C++ 1500,wx containers,compatible with 3.0)
running under Windows Server 2012 R2 (build 9600), 64-bit edition as appveyor, locale is C
!!! Non-GUI test failed.
Command exited with code -1073741819
i.e. it crashes somewhere without any other information. I can't reproduce this locally, I'll try to find some way to get a stack trace from AppVeyor but if I don't manage to do it quickly, I'll just disable the tests there as I really want to merge this, finally, instead of postponing it yet again.

Also have no idea why this happens, just wanted to let you know that it's possible to connect via remote desktop to appveyor. I don't remember how it works, but it was documented in the appveyor documentation. Maybe it could give some insight.

MaartenBent · 2021-01-16T01:08:14Z

I am getting the segfault:

 	wxbase315ud_net_vc.dll!wxEvtHandler::CallAfter<`anonymous namespace'::StateEventProcessor>(const `anonymous-namespace'::StateEventProcessor & fn) Line 3724	C++
>	wxbase315ud_net_vc.dll!wxWebRequestImpl::SetState(wxWebRequest::State state, const wxString & failMsg) Line 196	C++
 	wxbase315ud_net_vc.dll!wxWebRequestWinHTTP::SetFailed(unsigned long errorCode) Line 274	C++
 	wxbase315ud_net_vc.dll!wxWebRequestWinHTTP::HandleCallback(unsigned long dwInternetStatus, void * lpvStatusInformation, unsigned long dwStatusInformationLength) Line 196	C++
 	wxbase315ud_net_vc.dll!wxRequestStatusCallback(void * __formal, unsigned __int64 dwContext, unsigned long dwInternetStatus, void * lpvStatusInformation, unsigned long dwStatusInformationLength) Line 119	C++

m_handler is invalid in wxWebRequestImpl::SetState.

And seems to be caused by WebRequest::Cancel test case.

MaartenBent · 2021-01-16T01:56:31Z

So I traced it down to calling SetState(wxWebRequest::State_Cancelled) in wxWebRequestWinHTTP::Cancel().
This causes the RunLoopWithTimeout() in the WebRequest::Cancel test case to stop early, and destructing the wxWebRequest.
After this, a wxRequestStatusCallback arrives trying to access the destructed webRequest.

Notice that this would also generate two wxWebRequest::State_Cancelled events, one in wxWebRequestWinHTTP::Cancel() and one when WINHTTP_CALLBACK_STATUS_REQUEST_ERROR callback is received.

vadz · 2021-01-16T12:08:34Z

Thanks! This is indeed wrong, and I can reproduce this if I insert some sleep calls into the worker thread (but still not, weirdly, without it).

Anyhow, I think:

State should only be set to State_Cancelled when the request is really cancelled.
We should ignore state changes when the state already has this value in SetState(). Ideally this shouldn't happen at all, in fact, but I'm 100% sure it doesn't...

Add a trivial wxWinHTTPCloseHandle() wrapper calling wxLogLastError() if closing the handle failed -- this is really not expected to happen, so make sure to at least log it if it does.

Under MSW, don't set the state to State_Cancelled as soon as Cancel() was called, as the request was still used from the other threads afterwards, resulting in race conditions and crashes. Fix this by just removing the SetState(State_Cancelled) call from the main thread, as it was redundant anyhow. This also makes the behaviour correspond to the documentation, which indicates that Cancel() works asynchronously. Also ensure, for all backends, that we actually cancel the request only once, even if public Cancel() is called multiple times. This required renaming the existing wxWebRequestImpl::Cancel() to DoCancel().

We shouldn't call SetState() to switch to the state that we're currently already in, normally. Add an assert to verify that this indeed doesn't happen. Also improve the logging statement to show both the old and the new states.

vadz · 2021-01-16T13:05:51Z

Unfortunately I've found another problem which I think needs to be fixed now: wxWebRequest::GetNativeHandle() is not very useful because we can't use it at the right moment. It can't be used before calling Start() because it may return 0 (there is a discrepancy between the backends here: CURL one allocates the native handle immediately, but the two other ones do it in Start(), and I think we might actually want to postpone initialization until Start() in wxWebRequestCURL too). And once Start() is called, it's too late because the request may have already executed asynchronously before the main thread has time to do anything.

The simplest solution I can see is to handle State_Active specially in SetState() and process the event immediately. This would allow to execute the code doing something with the native handle just before actually starting it.

Any better ideas/objections to this one?

This is more consistent with the other backends, which all change the state before actually launching the asynchronous request.

Semantics of this function wasn't really clear and it was used only once, so just inline it at the point of use and define better what happens for various states there. Also use a switch rather than testing for individual states to make sure this code is updated if another state is added in the future. No real changes.

Call GetResponse() only once. Also put the code for State_Completed inside the case for this state in the switch instead of testing for it separately later. No real changes.

This is required in order to allow doing something with the request when it already have a valid native handle, but hasn't actually started yet.

This state can never be returned to, once the state becomes active.

Check that GetNativeHandle() behaves as documented.

No real changes, just try to organize the code in a more logical order.

vadz · 2021-01-16T14:09:44Z

With the latest changes I can call curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); when the state becomes State_Active to show verbose messages, so I think now it's good enough.

I'll merge this tomorrow (sorry for the short delay, but I might be unavailable next week) if there are no objections.

Hopefully this might provide more information about the failure of this test in MSVS 2008 build on AppVeyor.

vadz · 2021-01-16T20:29:31Z

~~There is still one test failure and I still can't reproduce it.~~ This one seems to have been a fluke.

I've also found another problem: wxEVT_WEBREQUEST_DATA is currently dispatched in the worker thread. This is quite unexpected but OTOH I'm not sure how are we supposed to send it to the main thread without copying the data (which is something we definitely don't want to do). And it's interesting that OnRequestData() in the sample is already written in a way safe to use in a thread and, notably, uses CallAfter() in it. So I wonder if it was really meant to behave in this way? If so, we must document that it's called in another thread. I also still have lingering doubts about thread-safety here...

No real changes, just make sure we use an (exhaustive) switch rather than a less obvious sequence of if statements.

This event was processed in a worker thread, which was different from all the other events and also almost surely not thread-safe, so change this and queue it for processing in the main thread instead. Use wxMemoryBuffer instead of non-owning pointer in wxWebRequestEvent and reset the buffer used internally every time to ensure the data is still available by the time the event is processed. Also increase the amount of data downloaded in the "advanced" page of the sample as it has to be greater than wxWEBREQUEST_BUFFER_SIZE, which is currently 64KiB, to have a chance of seeing the value actually change, otherwise all the data arrives in a single event. As it is, using the maximal size supported by the httpbin service, we only get 2 events.

vadz · 2021-01-16T23:05:50Z

I've changed the code to process wxEVT_WEBREQUEST_DATA in the main thread and now I think I'm really, really ready to merge it.

Any comments/reviews and even reminders of something important I forgot to do are still very welcome, of course!

mtangoo · 2021-02-11T17:07:54Z

Congratulations to everyone who made this possible.

vadz reviewed Oct 14, 2018

View reviewed changes

TcT2k force-pushed the http_request branch from 3b2ad2a to 75368ed Compare October 15, 2018 08:55

vadz reviewed Oct 15, 2018

View reviewed changes

eranif reviewed Oct 15, 2018

View reviewed changes

Comment thread interface/wx/httprequest.h Outdated

catalinr suggested changes Oct 17, 2018

View reviewed changes

TcT2k force-pushed the http_request branch 2 times, most recently from 4a7599e to 08fcaf2 Compare October 17, 2018 09:30

catalinr suggested changes Oct 17, 2018

View reviewed changes

Comment thread interface/wx/webrequest.h Outdated

Comment thread interface/wx/webrequest.h Outdated

Comment thread interface/wx/webrequest.h Outdated

catalinr suggested changes Oct 17, 2018

View reviewed changes

Comment thread interface/wx/webrequest.h Outdated

Comment thread interface/wx/webrequest.h Outdated

MaartenBent reviewed Oct 18, 2018

View reviewed changes

Comment thread samples/webrequest/webrequest.cpp Outdated

TcT2k changed the title ~~Modern HTTP(s) Support [Proposal]~~ Modern HTTP(s) Support Oct 19, 2018

TcT2k force-pushed the http_request branch from 946d94a to a2712ad Compare October 20, 2018 22:28

TcT2k force-pushed the http_request branch 9 times, most recently from baf9af2 to 508287e Compare October 26, 2018 19:07

TcT2k force-pushed the http_request branch from 508287e to 4d4fcff Compare October 29, 2018 20:16

vadz added 3 commits January 15, 2021 23:48

Remove semi-public wxWebSession::GetImpl()

a561cf1

It's better not to have this method in the public class, even if it means that we need to pass a wxWebSessionImpl object to wxWebRequestImpl ctor explicitly now. No real changes.

Add GetNativeHandle() to wxWebSession and wxWebRequest

65aad89

This allows to retrieve the handles used internally in order to do something not supported by the public API yet.

vadz added 3 commits January 16, 2021 13:47

Check for errors when calling WinHttpCloseHandle()

b428e53

Add a trivial wxWinHTTPCloseHandle() wrapper calling wxLogLastError() if closing the handle failed -- this is really not expected to happen, so make sure to at least log it if it does.

Check that SetState() always actually switches states

5babe57

We shouldn't call SetState() to switch to the state that we're currently already in, normally. Add an assert to verify that this indeed doesn't happen. Also improve the logging statement to show both the old and the new states.

vadz added 7 commits January 16, 2021 14:40

Set state to Active slightly earlier in wxWebRequestWinHTTP

a52f353

This is more consistent with the other backends, which all change the state before actually launching the asynchronous request.

Slightly refactor wxWebRequestImpl::ProcessStateEvent()

1bf6d5e

Call GetResponse() only once. Also put the code for State_Completed inside the case for this state in the switch instead of testing for it separately later. No real changes.

Process event about the request becoming active synchronously

3241c44

This is required in order to allow doing something with the request when it already have a valid native handle, but hasn't actually started yet.

Check that we never get events about switching to State_Idle

204ae59

This state can never be returned to, once the state becomes active.

Verify that native handle is available when state becomes active

0742ae9

Check that GetNativeHandle() behaves as documented.

Rearrange states in the switch in a more logical order

0777fda

No real changes, just try to organize the code in a more logical order.

Show error description if available when Cancel() test fails

c41041a

Hopefully this might provide more information about the failure of this test in MSVS 2008 build on AppVeyor.

vadz added 2 commits January 16, 2021 23:33

Use switch over enum-valued variable rather than "if"s

4c19572

No real changes, just make sure we use an (exhaustive) switch rather than a less obvious sequence of if statements.

vadz merged commit febd185 into wxWidgets:master Jan 17, 2021

gonosztopi mentioned this pull request Jan 24, 2021

Use libcurl for HTTP transfers amule-project/amule#218

Closed

5 tasks

NPDevelopmentCode mentioned this pull request Jan 25, 2021

Remove the worker thread from wxWebSessionCURL #2193

Merged

TcT2k deleted the http_request branch April 7, 2021 14:26

Uh oh!

Conversation

TcT2k commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sample App Screenshots

Uh oh!

oneeyeman1 commented Oct 14, 2018

Uh oh!

eranif commented Oct 14, 2018

Uh oh!

vadz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TcT2k commented Oct 14, 2018

Uh oh!

vadz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TcT2k commented Oct 16, 2018

Uh oh!

catalinr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

catalinr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

catalinr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtangoo commented Oct 21, 2018

Uh oh!

TcT2k commented Oct 31, 2018

Uh oh!

MaartenBent commented Oct 31, 2018

Uh oh!

TcT2k commented Oct 31, 2018

Uh oh!

vadz commented Jan 15, 2021

Uh oh!

TcT2k commented Jan 15, 2021

Uh oh!

MaartenBent commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaartenBent commented Jan 16, 2021

TcT2k commented Oct 14, 2018 •

edited

Loading

MaartenBent commented Jan 16, 2021 •

edited

Loading

vadz commented Jan 16, 2021 •

edited

Loading