HTTP.jl v0.8.0

[EricForgy]

Made some good progress today. All tests are passing except

• Throttling (throttling_test.jl)
• Error Handling (error_test.jl)

Would it make sense to merge this to a new branch so I / we can submit PR to the new branch instead of master?

[coveralls]

Coverage decreased (-2.4%) to 77.273% when pulling 0338defda9c51e135fcbbccb6c53d155b4ff5f27 on EricForgy:HTTPv0.8.0 into b694930254ede76efce6940275c071eb59fc8879 on JuliaWeb:master.

[coveralls]

Coverage increased (+14.4%) to 94.041% when pulling 8e52ba705d23b3ad3cd110453076e4a63fadc6f8 on EricForgy:HTTPv0.8.0 into b694930254ede76efce6940275c071eb59fc8879 on JuliaWeb:master.

[EricForgy]

Yay! Error tests are passing now 🎉

[EricForgy]

@hustf 👋

Do you think it is worth the effort to resurrect the throttling tests?

All throttling has now been relegated to HTTP.jl (no need to check rate limits twice I think).

Edit: To be honest, I'm not very motivated to fix those tests. Having 1,038 tests run AND PASS is sufficient for me 😅

[hustf]

Eric, sorry for my sudden disappearance and reluctance to state opinions while not being able to put in the work.

Throttling: We made our own throttling function here because a) The interface was confusing and lead to hard-to-find errors. Hence the input check. I don't want to refer that uncool function in HTTP again. However, HTTP has also removed using this function as default. It just exists without being referred. b) I could not see the point of the way it actually works, but just because I don't see the point doesn't mean there is none. Now, issue #124 has been open for a while, and nobody has put in any defense of it. c) The function was not, as far as I could see, fully tested in HTTP, but we were using it. I was not comfortable with that.

Since this is one of those things we can do separately from the HTTP 0.8 upgrade, I'll remove the checkratelimit! and release a minor version.

[hustf]

Ok, there's a PR in now for a minor version, where the ratelimiter! function is removed, and the default ratelimiter function returns true always. There's inline documentation on how to improvise rate limiting. Come to think of it, I'll add a note in README.MD also.

[hustf]

That done... We'll follow your suggestion ( and delete branch issue130). I also got a few other things out of the way, so there's a chance I can review during the coming week.

If you have any local commits waiting now, please commit!

[EricForgy]

Sounds like a plan @hustf 👍

One thing you'll want to look at is the new Base.Close(ws::WSServer).

You had it set up to close the server when you write to WSServer.in, but that introduced some tasks that didn't seem necessary, so now you just close(wsserver) and it closes.

If this is ok with you, we can remove WSServer.in since it isn't used in my PR.

[hustf]

Hi, I kind of like the way closing the server works with the .in channel. But yes, it generates yet another task for the scheduler. I have believed that is virtually without cost in Julia. But let's come back to that. I need to study the code now.

In the meantime, could you provide your reasoning behind reverting HTTP to a mutable Server type? It has some downsides which I believe will slow down listenloop; things to do with thread safety and possibly warm-up-time. This has probably been discussed elsewhere, could you point to the relevant issue? If a mutable Server is a better approach, why would HTTP go away from that in the first instance?

[EricForgy]

Hi @hustf

Thanks for your comments.

In the meantime, could you provide your reasoning behind reverting HTTP to a mutable Server type?

When the ServerWS.server property is nothing, listen creates a new server and assigns it to ServerWS.server so I made it mutable. close(ServerWS) sets the property back to nothing. There is probably a better way to reactivate a closed TCPServer, but I didn't know how, so this was my solution. Open to suggestions :)

With some finessing, we can probably make it a struct instead of a mutable struct, but I'm not sure it will make much of a difference. As I described in the PR here:

https://github.com/JuliaWeb/HTTP.jl/pull/396#issuecomment-471240658

the struct is getting instantiated once whenever listen is called anyway, so I just moved it outside the function.

As it stands, accessing some properties are done inside the loop, but that could be changed easily enough too if it makes a difference.

As always, I'm happy to get feedback and open to suggestions 🙏

PS: I didn't realize WebSockets.HTTP.jl was being used in the wild yet. I thought it was still under developement, so I should changed WSServer back to ServerWS.

[hustf]

Thanks for the explanation. With your approach, there may no longer be a need to store both a reference to the task (for some error messages), as well as to the server (for other error messages, and for closing). Except for perhaps error tracking. I don't have more time today, but I'm curious as to how you solved that.

As per now, there are some trivial test failures to weed out. When everything is working, it's time to see what we'll keep (yes, I noticed the unnecessary name change), and consider if more deprecations are in order.

If you have more changes, please open a PR to branch HTTP0.8! We'll accept those immediately, and as you may have seen there's a temporary PR in place where the branch is tested.

[hustf]

P.S. Regarding the PR to HTTP. I'm not sure that's a fluke, having just skimmed the output. There has been some bugfixes to Julia 0.7 and Julia 1.0. Perhaps you need to update the install script to get the latest version, or perhaps the bugfixes weren't actually pushed to the little-used x86 version.

[EricForgy]

Hi @hustf

As per now, there are some trivial test failures to weed out.

Where do you see that? This PR had no test failures. Are you talking about WebSockets.jl or HTTP.jl?

P.S. Regarding the PR to HTTP. I'm not sure that's a fluke, having just skimmed the output. There has been some bugfixes to Julia 0.7 and Julia 1.0. Perhaps you need to update the install script to get the latest version, or perhaps the bugfixes weren't actually pushed to the little-used x86 version.

Yeah. I'm not sure what is going on there. The first time I ran the tests, 7 of 8 environments passed. The one that failed was nightly x86. I suspected it was a fluke so forced a commit to trigger the tests again with no change to the code. This time, again, 7 of 8 environments passed INCLUDING the nightly x86 test. The failing test this time was v1.0 x86. Kind of strange 🤔

[hustf]

Hi, the mentioned test failures are in branch HTTP0.8. They are likely due to the choices I made when merging this PR into the new branch and resolving the conflicts. The new branch was copied from master after we made the release removing checkratelimit!.

Regarding the x86 failures. The CI computers are notoriously slow. If you're desperate, try the option sleeptime = ..., or try to provoke compilation before actual tests. I don't know what's the issue there, but there's usually some reason.

[EricForgy]

Hi, the mentioned test failures are in branch HTTP0.8.

Ah. Ok. I'll see if I can help fix HTTP0.8 👍🙏

I've been working on WebIO.jl. I need it to run with HTTP.jl v0.8.0 as well, but it is not so easy. I've already got the simple HTTP stuff working, but the tests rely on Mux, Blink and iJulia so all of them need to be updated before the tests will pass.