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

Http.jl #88

Merged
merged 8 commits into from
Feb 16, 2018
Merged

Http.jl #88

merged 8 commits into from
Feb 16, 2018

Conversation

EricForgy
Copy link
Contributor

Extending #87 with tests passing.

@EricForgy EricForgy mentioned this pull request Feb 14, 2018
@hustf
Copy link
Collaborator

hustf commented Feb 14, 2018

For the development branch, I suggest we merge the PRs very quickly, then review the branch as a whole since there will probably be some to and fro. This one has some conflicts that you will probably want to resolve first.

I have three questions after just skimming parts of the code. They are not well thought out, but it may be better to put them to you now to avoid possible double work.

  1. Will users be able to open websockets without performing the handshake? A tempting workflow might be to use websockets for internal communcation between processes. In deed that will be useful. But if someone does that, they are also open to attacks from, well, anywhere in some cases, something we probably don't want to encourage.

  2. Can we write using AbstractStrings? That includes substrings. Which can save time by not copying whole blocks of memory. Since Javascript loves strings, a lot of the traffic on websockets will be strings, and the websockets are bound to become the bottleneck in any such application. Examples ought to include the best practice.

  3. Can we treat strings differently than arrays? Strings don't change while occupying the same memory block (not sure if that applies to AbstractStrings), while arrays can be changed by other processes.

You will only see this tomorrow, of course, but happy Valentines to you and your wife!

@EricForgy
Copy link
Contributor Author

  1. Will users be able to open websockets without performing the handshake? A tempting workflow might be to use websockets for internal communcation between processes. In deed that will be useful. But if someone does that, they are also open to attacks from, well, anywhere in some cases, something we probably don't want to encourage.

No. We should strive to be 100% compliant with the IETF specs.

https://tools.ietf.org/html/rfc6455

A WebSocket package must always check for a handshake.

I suspect you might be thinking of the masks. A WebSocket client must always mask the payload when communicating with a server. This is for security reasons I don't fully understand (I'm still new too, but have spent a good chunk of time with the IETF spec recently).

HOWEVER, a WebSocket server must NOT mask the payload. The original WebSockets would throw an error if the frame used a mask, but that means you could not use the package as a client. I've taken this out so we can use this package as both client and server (even simultaneously). That is also why I added the field server to the WebSocket object. This idea is borrowed from @samoconnor's HTTP.WebSockets.jl.

We should check that a message sent from a server is not masked and a message sent from a client is masked and throw an error otherwise. If this is not currently implemented, it should be and I'll add it.

  1. Can we write using AbstractStrings? That includes substrings. Which can save time by not copying whole blocks of memory. Since Javascript loves strings, a lot of the traffic on websockets will be strings, and the websockets are bound to become the bottleneck in any such application. Examples ought to include the best practice.

Sure. That would be no problem. WebSocket frames can represent either text or binary data (but both are just bytes). The original WebSockets.jl had two write methods: one with a String which assumes you want to send text and one with Vector{UInt8} that assumes you want to send binary. We can and should have a write method that can accept a byte array and still send it as text by specifying an opcode. This is possible in HTTP.WebSockets.jl and I could port that functionality here easily enough. WebSockets are all about sending bytes and don't care what the bytes represent. That is up to you.

  1. Can we treat strings differently than arrays? Strings don't change while occupying the same memory block (not sure if that applies to AbstractStrings), while arrays can be changed by other processes.

Yes. Sure. We can treat them however you like. Open to suggestions. I feel half-way competent to implement some of this stuff now. My stuff may be in hackish form, but should hopefully clean up nice with help once working (two language problem in action).

You will only see this tomorrow, of course, but happy Valentines to you and your wife!

Ha! Thanks. Was too excited. Had to reply before sleeping 😊 Happy Valentine's Day! 💟

@EricForgy
Copy link
Contributor Author

EricForgy commented Feb 15, 2018

Both examples: chat.jl and server.jl are working now 👍

@hustf
Copy link
Collaborator

hustf commented Feb 15, 2018

Will you be able to resolve the conflicts? I may be able to give a hand in the weekend.

@hustf
Copy link
Collaborator

hustf commented Feb 15, 2018

Regarding my questions and your replies regarding strings. I had a go at something like it already, but my solution was slower than the current in the end. Also, 0.7 will open some new possibilities. So let's leave this speed improvement out for now is what I suggest.

@hustf hustf merged commit 1dd534b into JuliaWeb:change_dependencies Feb 16, 2018
hustf added a commit that referenced this pull request Jun 6, 2018
* Use HTTP.jl (#87)

* Use HTTP.jl

* Support SSLContext

* Http.jl (#88)

* Use HTTP.jl

* Support SSLContext

* Get tests to pass

* Fix chat example

* examples/server.jl is working.

* Restrict tests to Julia v0.6 (v0.5 no longer supported).

* Almost done. Last test is nearly passing...

* Http.jl (#89)

* Use HTTP.jl

* Support SSLContext

* Get tests to pass

* Fix chat example

* examples/server.jl is working.

* Restrict tests to Julia v0.6 (v0.5 no longer supported).

* Almost done. Last test is nearly passing...

* Fix REQUIRE for appveyor / travis

* Add test/REQUIRE

* Update README, graceful close, add ping/pong. Temp tests. (#90)

* Update README

* Graceful close from client + run HTTP server HttpServer server simulataneously.

* Test ping, pong. Temp tests.

* new file:   examples/chat_explore.html
new file:   examples/chat_explore.jl

* modified:   appveyor.yml  - also test change_dependencies branch. Remove when merging to master.

* modified:   README.md - temporary badges for CI tests on appveyor.

* Http.jl (#92)

* Update README

* Graceful close from client + run HTTP server HttpServer server simulataneously.

* Test ping, pong. Temp tests.

* Add try, catch and close

Indent

WIP

* Fix String mask issue

* Minor: Fix an indent

* Update WebSockets.jl

Add a comment to remove the `copy` for v0.7.

* modified:   examples/chat_explore.jl - extended with HTTP server and a modifified upgrade function (get errors)

* Only copy String bytes if masked (#93)

* modified:   examples/chat_explore.jl - changed interface to gatekeeper, import functions where necessary.

* modified:   examples/chat_explore.html
modified:   examples/chat_explore.jl

* mod:    examples/chat-client.html
mod:    examples/chat_explore.html
mod:    examples/repl-client.html
mod:    test/browsertest.html
mod:    test/browsertest2.html
        - tightened html5 syntax, now enforced by Firefox
mod:    examples/chat_explore.jl
		- removed ad-hoc modification of WebSockets.upgrade
mod:    src/HTTP.jl
		- slightly quicker is_websocket_handshake, added comment
mod:    src/HttpServer.jl
		- slightly quicker is_upgrade, added comment
		- revised upgrade function, improved feedback on caught errors,
		  otherwise as previously in chat_explore.jl
		  This may be changed back again if we include more fields in the
		  WebSocket type.

* mod:   src/HTTP.jl
		Droppped some temporary debug code that remained by accident.
		Faster checking of
			Connection => "upgrade" || "keep alive, upgrade"
mod:   src/HttpServer.jl
		Faster checking of
			Connection => "upgrade" || "keep alive, upgrade"

* modified:   src/HTTP.jl    # Fix bug: HTTP.Request has no .status field.

* modified:   src/HTTP.jl        fixed last fix... Request-> Response of course.
                               upgrade: dropped calling handler taking the binary argument
modified:   src/HttpServer.jl  regexp matching "Connection => xxUPgradEx"

* 	modified:   src/HTTP.jl
	modified:   src/HttpServer.jl
	modified:   src/WebSockets.jl

* 	modified:   src/HTTP.jl       Expanced comment on 'upgrade', removed debug 'warn' lines, removed 'listen',
	                              removed 'binary' argument from functions upgrade and open.
	modified:   src/HttpServer.jl # Temporary comment on handshakes, removed double try-catch on close.
    modified:   src/WebSockets.jl # TODO comment on error handling

* 	modified:   README.md      Testing include a picture.
	new file:   examples/serve_verbose/svg/ws_neighborhood.svg

* mod:   README.md     Image experiment
mod:   examples/serve_verbose/svg/ws_neighborhood.svg  Smaller size

* modified:   README.md      Took away temporary network graph (github stylesheets mess with inlined svg)
modified:   src/HttpServer.jl  Reinstated try-catch after ci failure (this works locally)

* new file:   benchmark/REQUIRE			No direct effect, lists
										extra requirements for
										benchmark
new file:   benchmark/bce.html			Browser Client Echo definition
new file:   benchmark/benchmark.jl		Comment lines only, so far
new file:   benchmark/benchmark_prepare.jl
										Bandwidth tests, all clients
new file:   benchmark/favicon.ico		Tab icon for bce.html
new file:   benchmark/functions_benchmark.jl
										Included in benchmark_prepare.jl
new file:   benchmark/functions_open_browsers.jl
										Improved functionality, will replace
										test/functions_open_browsers.jl
new file:   benchmark/logs/benchmark_results_readable.log
										Manual copy-paste from generated
										benchmark_results.log
										...since the utf8-encoding to file
										is not working well.
new file:   benchmark/phantom.js		Definitions for phantomjs browser
new file:   benchmark/ws_hts.jl			Server for HTTP, websockets, bce.html
new file:   benchmark/ws_jce.jl			Julia Client Echo
new file:   logutils/log_http.jl		Included in logutils_ws, HTTP types
new file:   logutils/log_httpserver.jl	Included in logutils_ws, HttpServer types
new file:   logutils/log_ws.jl			Included in logutils_ws, WebSocket types
new file:   logutils/logutils_ws.jl		Logging utilities, included in
										benchmark_prepare.
modified:   src/HTTP.jl					Added help text examples.
										In open(), add own error handling
										(to be improved). upgrade() error
										handling is also temporary.
modified:   src/WebSockets.jl			Non-blocking read while closing.
										Added a reasonable closing handshake
										timeout
										Commented out WebSocketClosedError (this
										must be considered more): Breaks some test
										code.
										Rewriting comments (not finished)
										In read(ws), ignore most easy-to-trigger errors
										which have to do with opening / closing /
										scheduling/ browser behaviour.
modified:   test/runtests.jl			Sleep a little longer, CI is slow.

* modified:   benchmark/functions_benchmark.jl    Improved explanations,
                                                dropped dead code densityplot
modified:   benchmark/ws_hts.jl                 Added testing for pre-existing
                                                sockets.
modified:   benchmark/ws_jce.jl                 Added explanation for currently unused 'delay' code
modified:   examples/chat_explore.jl            Added reference to alternative close-down method

* Do not return control frames like ping.
Recurse to the next frame like in master.

modified:   src/WebSockets.jl

 Reintroduce recursion on control frames:376. Returning control codes
 would crash String(msg). Multi-frame messages are also possible.
 Add WebSocketClosedError messages :47, for underlying exceptions.
 Propagate unrecognized errors.

 Remove INFO message when closing. Readguarded() informs better.

 Added functions:
  readguarded          return tuple: data and success indication
                       incomplete messages are always empty
  writeguarded         return success indication
  subprotocol(request) common for Httpserver and HTTP
  target(request)      common for Httpserver and HTTP
  origin(request)      common for Httpserver and HTTP

modified:   src/HTTP.jl

 make upgrade(ws) call websocket handler with full request.
     Origin can now be determined, as per recommendations.
 call showerror with catch_stacktrace(). backtrace was less interesting.
 add method target(request)
 add method origin(request)
 add method subprotocol(request)
 add ServerWS
     WebsocketHandler
     serve(::ServerWS, etc..)
     The intententions is user code brevity, access to HTTP
     keyword options, more similar interface with HttpServer,
     can save the user from distinguishing Stream and Message.

modified:   src/HttpServer.jl

 add method target(request)
 add method origin(request)
 add method subprotocol(request)
 improve inline doc for WebSocketHandler

modified:   examples/chat_explore.jl
 demonstrate new functions, improve readability

modified:   benchmark/functions_benchmark.jl
 capture WebSocketClosedError with readguarded(ws)

modified:   logutils/log_httpserver.jl
 explicitly import Request and Response from HttpServer

modified:   test/functions_server.jl
 rephrase inline docs, use subprotocol(request)

modified:   test/runtests.jl
 not rely on pong returning read(ws) call

* 	modified:   src/WebSockets.jl    reinstate check for present mask with error message

* Status codes implemented for closing handshake

	modified:   benchmark/ws_hts.jl
isclosed -> !isopen

	modified:   src/HTTP.jl
remove dead code 'browserclient'::bool

	modified:   src/HttpServer.jl
reshuffle order of handshake checks, now similar to HTTP.jl

	modified:   src/WebSockets.jl
close a socket with handshake and 1002: ProtocolError when incorrect
mask received.
include status code for sending close
include received status codes for receiving close
update inline doc introduction
remove implemented TODO comments

* 	modified:   benchmark/logs/benchmark_results_readable.log
 document an overall reduction in bandwidths, 15% since last
 benchmark result commit. More than expected.

    modified:   src/HttpServer.jl
 bugfix remaining since last commit

    modified:   src/WebSockets.jl
 improve inline doc maskswitch!
 close: bugfix statuscode 0, use hton instead of reverse

    modified:   test/runtests.jl
 reintroduce unit tests from master, now including client
 add close status code test
 add varying message length

* Expand HttpServer tests to HTTP

    modified:   src/HTTP.jl
 change req.resource -> req.target

	modified:   test/runtests.jl
 add testset  "Test length typemax(UInt16) + 1"
 include HTTP in handshake testsets

* Unit tests 93%, excluding browsertests

	modified:   src/HTTP.jl
separate _openstream(..) from open for test access
	modified:   src/WebSockets.jl
future improvements
	modified:   test/runtests.jl
	deleted:    test/HTTP.jl
	new file:   test/client_server_test.jl
	new file:   test/client_test.jl
	new file:   test/error_test.jl
	new file:   test/frametest.jl
	new file:   test/handshaketest.jl

* 	modified:   src/WebSockets.jl
	dead code remove

* Add sleep(3) after start async server for travis-test
	modified:   test/client_test.jl
	modified:   test/error_test.jl

* Server channels for exceptions and traces
More tests

mod: src/WebSockets.jl
  export WebSocketClosedError
  close keyword argument 'freereason'
  received close handshake throws WebSocketClosedError
  improve criterion for bad mask error
  include reasons in forced close handshakes

mod: src/HTTP.jl
 use channels for error handling on server side
 removed invisible exception warnings in 'listen'

mod: benchmark/logs/benchmark_results_readable.log
add:   benchmark/logs/benchmark_results_readable_previous.log
   12% slower client side, 10% faster server side

mod: benchmark/ws_hts.jl
mod: benchmark/ws_jce.jl
  import clog_notime

mod: logutils/log_http.jl
mod: logutils/log_httpserver.jl
mod: logutils/log_ws.jl
mod: test/handler_functions_events.jl
 remove dead code, tabs

mod: test/client_server_test.jl
  wait for istaskstarted
mod: test/client_test.jl
 less console info, more sleep
mod: test/error_test.jl
 test all exceptions and exception interfaces

mod: test/frametest.jl
 add unknown opcodes, multi-frame message,
 bad mask errors, peerless close, close frames

mod: test/handshaketest.jl
 reduce console info
mod: test/runtests.jl
 add sleep and total group

* Info during tests, more sleeping
mod:   test/client_test.jl
mod:   test/error_test.jl
mod:   test/runtests.jl

* mod:   README.md          Large rewrite
mod:   src/WebSockets.jl  Add 'end' in example

* modified:   README.md            Another spin
modified:   src/HTTP.jl          Improve inline docs
modified:   src/WebSockets.jl    RSV1 compression bit remarks
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.

2 participants