JuliaWeb / WebSockets.jl

A WebSockets library for Julia
MIT License
158 stars 57 forks source link

compatible with HTTP [1.1.0 - 1.6.0) #182

Closed anj00 closed 1 year ago

anj00 commented 1 year ago

I tried to update to HTTP 1.+

  1. I could only make tests running for HTTP 1.1.0 and 1.5.5 (I could imagine all inbetween will be running). And of course this code will not run with HTTP prior 1.1.0
  2. bumped WebSockets version to 1.6.0 as this is pretty major move
  3. some test somehow needed a sleep(1) to actually run.
  4. copied some (basic skeletons) functions from HTTP 0.9 to HTTP.jl
anj00 commented 1 year ago

I tried to update to HTTP 1.+

  1. I could only make tests running for HTTP 1.1.0 and 1.5.5 (I could imagine all inbetween will be running). And of course this code will not run with HTTP prior 1.1.0
  2. bumped WebSockets version to 1.6.0 as this is pretty major move
  3. some test somehow needed a sleep(1) to actually run.
  4. copied some (basic skeletons) functions from HTTP 0.9 to HTTP.jl
  5. bump julia version to 1.6 min (same as for HTTP)
  6. appveyor somehow has a bug and tests against 1.5 julia. explicitly put LTS and latest released
anj00 commented 1 year ago

I guess it needs more work. It does hang in tests (works here locally). So something is not right. If anyone can take a look or has an idea, that would be great

mind6 commented 1 year ago

So the CI tests are hanging on 32-bit x86, and Julia 1.8.3 and nightly? I will make a mini-PR with only show-test.jl modified, to see if the original version pass these tests

anj00 commented 1 year ago

Well, the problem looks to be on trying to do anything with the socket

readbytes!(ws.socket, ab) hangs I tried to use peek(ws.socket) before calling readbytes!. But peek hangs as well now.

Maybe there are other methods to understand that the socket is actually ready?

And the issue seem to be happening only here @info "Listen: Server side initiates message exchange." @info "Listen: Server side initiates message exchange. Close from within server side handler."

i.e. the tests there echows function is used.

mind6 commented 1 year ago

Well, the problem looks to be on trying to do anything with the socket

readbytes!(ws.socket, ab) hangs I tried to use peek(ws.socket) before calling readbytes!. But peek hangs as well now.

Maybe there are other methods to understand that the socket is actually ready?

And the issue seem to be happening only here @info "Listen: Server side initiates message exchange." @info "Listen: Server side initiates message exchange. Close from within server side handler."

i.e. the tests there echows function is used.

In HTTP 0.9, the CI tests behave the same way they do on my Windows 10 machine. Are your tests passing locally? Are you on Linux?

anj00 commented 1 year ago

tests pass locally I am on windows 10.

mind6 commented 1 year ago

tests pass locally I am on windows 10.

I wonder if HTTP 1.5 tests pass in CI, if you just made a spurious pull request to HTTP.jl 😆

anj00 commented 1 year ago

I only added a couple of printouts and now tests are good.

anj00 commented 1 year ago

Can someone with right access please comment on the changes? Accept the changes? So WebSockets can move on HTTP 1.5 and stop blocking other packages?

mind6 commented 1 year ago

Can someone with right access please comment on the changes? Accept the changes? So WebSockets can move on HTTP 1.5 and stop blocking other packages?

I think he's on vacation for a few days

hustf commented 1 year ago

Vocation this time... Tomorrow is WebSockets fun time. Looking forward to see how you solved this! Thanks specifically for pulling this through CI, which is sometimes arbitrary, sometimes evil.

hustf commented 1 year ago

Regarding test output to console: There's been a small explosion in the number of outputs. A lot of this is clutter from lower-level functionality, since the package HTTP now outputs debug statements in a different manner from before. We should have a look at cleaning up the clutter (through modifying 'shouldlog'), but for now it's probably better to keep this PR minimal.

fonsp commented 1 year ago

@hustf Awesome to see that this is getting picked up again! If you are interested, I would love to have a call about the position of WebSockets.jl after the recent overhaul of HTTP.WebSockets: https://github.com/JuliaWeb/HTTP.jl/pull/843 . It would be great if we could centralise documentation and issues. You can reach me at: fons@plutojl.org

hustf commented 1 year ago

Updated the long comment above.

For the open process: Should we exclude Julia 1.8.0 to 1.8.2? Would lazy updaters benefit from downloading a new version of WebSockets with deprecation or warning messages? Note that I have not checked if 1.8.2 would fully work.

Also note that since this PR seems to be working well, I'll also have a look and smell at the code before merging.

mind6 commented 1 year ago
Abrupt close & error handling: Test Failed at C:\Users\frohu_h4g8g6y\.julia\environments\webs\WebSockets.jl\test\error_test.jl:118
  Expression: err.message == "while read(ws|server) Client side closed socket connection - Performed closing handshake."   Evaluated: "while read(ws|server) Base.IOError(\"read: connection reset by peer (ECONNRESET)\", -4077)" == "while read(ws|server) Client side closed socket connection - Performed closing handshake."

These two errors look like the two errors when using HTTP 0.9

So far it's been oscillating a bit. Julia Ver. lines 118,146 tests
1.6.3 pass
1.6.7 fail
1.8.1 fail
1.8.2 pass
1.8.3 pass
nightly pass

Well the trend is good. We can call it a pass if we want. 😁

p.s. I think the people wanting to update, want the latest packages with HTTP on latest Julia. Requiring 1.8.3 is fine if that is truly stable. Someone still on LTS may not care about HTTP 1.

anj00 commented 1 year ago

I realized that handler function interface for HTTP was changed for no good reason. so rolled it back to original and moved details inside the module. So users would have easier time migrating.

I believe the migration is only going from this

WebSockets.ServerWS(
    HTTP.RequestHandlerFunction(test_handler),
    WebSockets.WSHandlerFunction(test_wshandler))

To this

WebSockets.ServerWS(
    WebSockets.RequestHandlerFunction(test_handler),
    WebSockets.WSHandlerFunction(test_wshandler))

Probably needs to be put in readme?``

hustf commented 1 year ago

After commit fc55503, examples/chat_explore.jl works again. So that syntax is still OK.

But README.md and Project.toml needs an update, possibly other files do as well.

I will call this PR successful and merge it to master, so that it will be easier for @mind6 to contribute, and people who are not interested in websockets per se can checkout master. This is no doubt an improvement over the current master's state.

Also, when CI is run from Websockets master, I believe the job will be submitted with different ownership. I was not able to cancel and restart hung jobs from this PR. Probably, anj00 has that access too.

Oh, and by the way: Thank you very much!

hustf commented 1 year ago

I believe the migration is only going from this...

There were two issues with re-running the old README example.

1) WebSocketLogger was not in scope. The error message was captured by HTTP.jl somewhere, as the with_logger call was compiled. 2) HTTP no longer accepted Response(200) as valid. Response(200, OK) works. Instead of posting an issue with HTTP.jl, I went ahead and updated our code.