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

KTOR-5199 Support WebSockets in Curl engine #3950

Open
wants to merge 26 commits into
base: 3.1.0-eap
Choose a base branch
from

Conversation

dtretyakov
Copy link
Contributor

Subsystem
Client, Curl engine

Motivation
Curl 7.86.0 added experimental support for WebSockets.

Solution
This PR brings support of experimental WebSockets in libcurl KTOR-5199.

To verify WebSockets availability we're using curl_version_info which returns list of enabled protocols.

The CurlResponseBodyData become interface with CurlHttpResponseBody/CurlWebSocketResponseBody implementations.

The bodyStartedReceiving is used to detect the end of headers section.

We're using WebSocket with callbacks approach where:

  • curl_ws_send used to send outgoing frames
  • onBodyChunkReceived with curl_ws_meta used to receive incoming frames

Environment
Since WebSockets feature is experimental we need to enable them during curl compliation.

macOS

curl https://raw.githubusercontent.com/dtretyakov/homebrew-cloudflare/master/curl.rb -o curl.rb
brew install -s curl.rb 

linux

Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Hey @dtretyakov, thanks for the PR.

Please let me know when you have an update about the Curl binary version. I will run tests on CI

@DRSchlaubi
Copy link
Contributor

DRSchlaubi commented Jan 24, 2024

Trying to connect to wss://gateway.discord.gg/?v=10&encoding=json&compress=zlib-stream is returning 400 Bad request so there are some issues with this implementation`

After some further debugging the Sec-WebSocket-Key and Sec-WebSocket-Version headers are set twice, once by curl and once by ktor, removing them form ktor solves the 400 Bad request issue

New issue curl_multi_perform seems to block until the connection is done, making any additional requests impossible

DRSchlaubi

This comment was marked as duplicate.

@DRSchlaubi

This comment was marked as outdated.

@dtretyakov
Copy link
Contributor Author

@DRSchlaubi thanks a lot for verification and review.

New issue curl_multi_perform seems to block until the connection is done, making any additional requests impossible

Maybe you could share a test case when it could be reproduced?

Trying to connect to wss://gateway.discord.gg/?v=10&encoding=json&compress=zlib-stream
Possible fix for duplicated header issue

Thanks, I added a list of WebSocket headers which are handled by cURL itself, so connection to this service should work.

@DRSchlaubi
Copy link
Contributor

DRSchlaubi commented Jan 25, 2024

Maybe you could share a test case when it could be reproduced?

Sure

fun main() = runBlocking { 
    val client = HttpClient { 
        install(WebSockets)
    }
    
    launch { 
        client.webSocket("wss://echo.websocket.org") {
            while(!incoming.isClosedForReceive) {
              // Keep connection alive
                incoming.receive()
            }
        }
    }
    
    println("Wait for websocket to connect")
    delay(10.seconds)
    println("Requesting now!")
    
    println(client.get("https://httpbin.org/200"))
}

@DRSchlaubi
Copy link
Contributor

DRSchlaubi commented Jan 25, 2024

After the latest changes most headers are not set, including "Authorization", which is not great

Fix in review comment

@DRSchlaubi
Copy link
Contributor

DRSchlaubi commented Jan 25, 2024

After more debugging it seems like the issue is this loop

    @OptIn(ExperimentalForeignApi::class)
    internal fun perform() {
        if (activeHandles.isEmpty()) return

        memScoped {
            val transfersRunning = alloc<IntVar>()
            do {
                println("Transfers running:${transfersRunning.value}")
                synchronized(easyHandlesToUnpauseLock) {
                    var handle = easyHandlesToUnpause.removeFirstOrNull()
                    while (handle != null) {
                        curl_easy_pause(handle, CURLPAUSE_CONT)
                        handle = easyHandlesToUnpause.removeFirstOrNull()
                    }
                }
                println("Running curl_multi_perform")
                curl_multi_perform(multiHandle, transfersRunning.ptr).verify()
                println("Ran curl_multi_perform")
                if (transfersRunning.value != 0) {
                    curl_multi_poll(multiHandle, null, 0.toUInt(), 10000, null).verify()
                    println("Ran curl_multi_poll")
                }
                if (transfersRunning.value < activeHandles.size) {
                    handleCompleted()
                    println("Ran complete")
                }
                println("Loop done")
            } while (transfersRunning.value != 0)
        }
    }

Because while (transfersRunning.value != 0) will remain true as long as the ws connection is still active

@dtretyakov dtretyakov force-pushed the curl/ws branch 2 times, most recently from 6e5aaf1 to 7b95cc5 Compare January 30, 2024 23:39
@dtretyakov
Copy link
Contributor Author

After more debugging it seems like the issue is this loop
Because while (transfersRunning.value != 0) will remain true as long as the ws connection is still active

@DRSchlaubi hopefully it was also addressed in the last commit, so you could try updating.

@DRSchlaubi
Copy link
Contributor

At first glance your new test case should cover this, will test myself tmr 👍

@DRSchlaubi
Copy link
Contributor

That indeed fixed it

@dtretyakov
Copy link
Contributor Author

Please let me know when you have an update about the Curl binary version. I will run tests on CI

Currently we're waiting when static linking of libcurl with it's dependencies will be in the repository for all targets and after that could merge the changes.

@DRSchlaubi
Copy link
Contributor

Any updates

@git-bruh
Copy link

Hey @dtretyakov , thanks a lot for your work on this PR. I noticed some strange behaviour when receiving payloads greater than 1024 bytes and it seems to be a libcurl quirk, have raised a PR here: dtretyakov#1

@bjhham bjhham requested a review from e5l November 8, 2024 13:53
@e5l e5l changed the base branch from main to 3.1.0-eap November 19, 2024 08:24
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Hey, @dtretyakov! Sorry for the long delay.
Could you rebase it on 3.1.0-eap branch before merging?

@DRSchlaubi
Copy link
Contributor

I actually have a branch which is based on 3.0.0 for kord, I could push that as it might be easier to migrate to 3.1.0 as 2.x

@DRSchlaubi
Copy link
Contributor

I pushed my work to curl/ws

marychatte and others added 9 commits December 12, 2024 11:16
* Support multiple client engines for JS/WasmJs
* CIO client Js/WasmJs support
* Enable CIO tests in client tests
* Make client engines `data object` to have `toString`
* Make ClientLoader work with multiple engines on js/wasmJs and so test CIO engine on nodejs(js+wasmJs)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Test libcurl build with conan

* Take both curl and openssl from the same build

* test no linux cache

* Move K/N linux cache disabling to just `ktor-client-curl` module
Some services use 403 instead of 401. Changing them might be impossible. With this change Ktor can flexibly work with any broken service.

---------

Co-authored-by: Osip Fatkullin <osip.fatkullin@jetbrains.com>
* KTOR-7679 Allow disabling body decoding on server
@dtretyakov dtretyakov force-pushed the curl/ws branch 2 times, most recently from 0a9f16f to f996e9d Compare December 19, 2024 06:08
@dtretyakov
Copy link
Contributor Author

dtretyakov commented Dec 19, 2024

Could you rebase it on 3.1.0-eap branch before merging?

It was rebased, but now we need to have a libcurl with websockets enabled:
#4445 (comment)

@dtretyakov
Copy link
Contributor Author

It looks that Conan libcurl receipt does no support option to enable WebSockets: conan-io/conan-center-index#26221

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.