export http_exception for non Windows builds

[JvdGlind]

If a project compiled with clang for macos links to CppRest, and the
-fvisibility=hidden compiler option is used, the http_exception is used
as std::exception. The clang compiler requires it to be exported in
order to know about the symbols.

Since the Visual Studio compiler throws warnings regarding the export of
a class that inherits from a non-exported class, std::exception in this
case, the WIN32 define check is added to omit this export.

[ghost]

All CLA requirements met.

[barcharcraz]

What error are you trying to resolve that required exporting this class. For what it's worth we don't recommend ever exporting an entire class, instead export the individual methods that you require. This will likely resolve the warnings as well

[JvdGlind]

Summary of what we did and how we figured out the issue was this:

We have been using CppRest for a couple of years now, and did so with the default visibility on MacOS (apple-clang). After a small research project on security, we found that all of our symbols were readable on MacOS while parsing them with tools like Radare, while they weren't visible on Windows (MSVC). We also had a fair amount of complaints about Mac developers not exporting the symbols, and thus breaking our Windows builds. We thus decided to turn on the -fvisibility=hidden flag for our apple-clang compiler.

After we managed to fix all compiler issues, we still had some other runtime issues, and one of our integration tests (network timeout handling) didn't finish, and was idling until our timelimit mechanism killed it. After some research we learned that we didn't get a web::http::http_exception (which we catch), but a std::exception (which we don't catch), and some undefined behavior happened, causing the application to idle. I am not entirely sure what causes it to idle instead of crash, but it might be due to the Boost test framework we use.

After some research from our Mac experts, we learned that apple-clang (also clang and gcc) needs to export (default visibility) their exception classes if they are caught in a different binary, in our case the test application. https://gcc.gnu.org/wiki/Visibility see "Problems with C++ exceptions (please read!)".

Since we use Conan to build our 3rd party libraries, including CppRest, we decided to append the following test to verify that the supplied test works (be sure to use -fvisiblity=hidden when compiling):

bool test_exception()
{
    auto fileStream = std::make_shared<ostream>();

    // Open stream to output file.
    pplx::task<void> requestTask = fstream::open_ostream(_XPLATSTR("results.html")).then([=](ostream outFile)
                                                                                 {
                                                                                     *fileStream = outFile;

                                                                                     web::http::client::http_client_config http_client_config;
                                                                                     http_client_config.set_timeout(std::chrono::microseconds(1));

                                                                                     // Create http_client to send the request.
                                                                                     http_client client(_XPLATSTR("http://127.0.0.1:310000"), http_client_config);

                                                                                     // Build request URI and start the request.
                                                                                     uri_builder builder(_XPLATSTR("/search"));
                                                                                     builder.append_query(_XPLATSTR("q"), _XPLATSTR("cpprestsdk github"));
                                                                                     return client.request(methods::GET, builder.to_string());
                                                                                 })

    // Handle response headers arriving.
    .then([=](http_response response)
          {
              printf("Received response status code:%u\n", response.status_code());

              // Write response body into the file.
              return response.body().read_to_end(fileStream->streambuf());
          })

    // Close the file stream.
    .then([=](size_t)
          {
              return fileStream->close();
          });

    // Wait for all the outstanding I/O to complete and handle any exceptions
    try
    {
        requestTask.wait();
        return false;
    }
    catch (const web::http::http_exception &e)
    {
        printf("Error exception:%s\n", e.what());
        return true;
    }
    catch (const std::exception &e)
    {
        printf("Error exception:%s\n", e.what());
        return false;
    }
    return false;
}

Regarding the exporting of a full class. My initial thought is that you really need the class symbol to be exported, rather than one or more members, but I could be wrong.

[barcharcraz]

The reason we (the vclibs team) discourage exporting entire classes on windows is just that it tends to lead to exporting more than you intended, and that's a big problem if you want to provide any kind of controlled ABI.

I'll ask around on the exception catching issue tomorrow. I suspect we won't end up needing to export the entire class.

[barcharcraz]

on windows there is no class symbol, it's just symbols for any static data members and any methods. There's also RTTI data but that's not affected by __declspec(dllexport) as far as I know (and does not result in a symbol in the DLL's export table anywaays)

[barcharcraz]

Thinking about this some more I'd prefer a new macro that's just for exporting type infos and is always disabled on windows. Clang does this and it's more clear, more searchable, and less likely to screw up doxygen.

maybe _ASYNCRTIMP_TYPEINFO and a comment in the macro definition relating to it's use.

[JvdGlind]

I updated by PR with your suggestion. It builds and works on our end (MSVC and apple-clang)

[barcharcraz]

I don't think clang is the correct thing to use here, it should be
"if _NO_ASYNCRTIMP is not defined and we're not building for windows"

[JvdGlind]

I was thinking about that, but since the clang compiler can also be used on Windows, I thought __clang__ would be better for cross compiler compatibility.

[barcharcraz]

ah, but the thing is you actually don't want to export the "class" when clang is running in msvc ABI mode (in this case it will define __clang__ and _WIN32.

[barcharcraz]

Does this work when _NO_ASYNCRTIMP is defined, it seems to me it might result in a duplicate definition. Also if you do define _NO_ASYNCRTIMP you should probably not define _ASYNCRTIMP_TYPEINFO on any platform

Sorry this has gone back and forth so much, I know it's annoying.

[barcharcraz]

If you considered this case and have a reason for structuring it this way that's fine, I just need to know why.

[JvdGlind]

The TYPEINFO define is within the !_NO_ASYNCTRIMP block. In the _NO_ASYNCTRIMP block I defined the empty case for all compilers, like _ASYNCTRIMP.

Since _ASYNCRTIMP_TYPEINFO needs to be defined for the compiler, I also defined an empty for WIN32 within the !_NO_ASYNCRTIMP block. I could move it out of the _NO_ASYNCRTIMP block and do something like:

    #if !_NO_ASYNCRTIMP && !defined(_WIN32)
    attribute
    #else
    empty
    #endif

But my initial thought was that the ASYNCRTIMP_TYPEINFO should be defined within the _NO_ASYNCRTIMP block for easier maintenance, as the developers then see that it is tightly linked to the export/no-export configuration.

[barcharcraz]

oh I'm sorry I missed the structure because of the whitespace, whoops

[barcharcraz]

Merged, sorry for the trouble with the macro placement.

[garethsb]

@JvdGlind I'm wondering why does this only apply to http_exception and not the many other exception types defined in C++ REST SDK (websocket_exception, task_canceled, etc.)?

[JvdGlind]

@garethsb We don't catch these exceptions explicitly in our codebase or testcases, so I don't know if it only applies to http_exception. I haven't taken the time to look into those, but it could very well be that those need an update as well.