-
Notifications
You must be signed in to change notification settings - Fork 57
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
Change_dependencies feedback #91
Comments
Oh, I forgot: @EricForgy. As part of the review, and also as a kind of template for assisting in modifying the test suite, I added a more verbose / explicit chat example. I'll ask you to kindly review it when we make a PR for merging the branch. It contains an alternative approach to doing without IDs. It also does away with JSON (which is very slow, this will improve with JSON2 and Julia 0.7). The slowness doesn't matter in examples, but it's nice to provide 'scaleable' examples. The motivation is otherwise listed at the top of chat_explore.jl Occasionaly I still manage to get untrappable errors from HttpServer. WebSockets is not part of the stack trace. I will report here if I manage to provoke it again; it would be lovely if we could modify HttpServer to trap the so-called errors. Next step for me will be to extend chat_explore.jl with a simultaneous HTTP port server. Then provide show(io, STDOUT) for the types defined here. |
I am super happy @samoconnor found the problem with the It should be much easier to write tests now and ultimately do some benchmarking / comparisons with HTTP.WebSockets. I'm super optimistic about the outcome of all this 👍 |
Not a comment tied directly to any particular lines of code now. Just a way to structure my own thoughts here. So my current understanding is this: We're always working on a single thread of execution. In normal code, we don't expect our tasks to be interrupted except when we sleep() or yield(). But when we're reacting to events like sockets opening or closing, the new tasks take immediate control. When e.g. a browser closes all of its tabs or we're restarting a server with several open tabs, all the tasks are just barely able to start before they're interrupted by the next close or open event. Browsers open lots of sockets very quickly, even when we're just typing in urls. And the same kind of thing happens while errors are being thrown. The call stack output is ready quite a while after the initial error event. So assigning place for a socket in a collection, and THEN putting in the reference to it is bound to cause trouble. That's one reason why httpserver sometimes just fails while opening sockets. Not a computer guy, but this is what I think is good practice just now: If we can't update our mutable reference collections in just one line, we shouldn't do it at all. In other words,
is fine. Some other solutions will cause random pain (glaring at HttpServer).
...cause random pain. We'll often find '2' way below in the logs. Hence, write to a temporary buffer, then output the buffer. |
Ref. WebSockets/HTTP.jl:33-37 and 58-61.
We are not giving the user the opportunity to write his own gatekeeper / selector functions, for example based on header "Sec-WebSocket-Protocol" (edit: Wrong. The user could define f in the context of the outer call, defining a 'nested function' in Julia terminology. In the outer call, http and binary is defined). A neater and more backwards-compatible style could be achieved by also accepting two- or three-argument f. For example:
But how to find out which arguments f take? I don't know any way to dispatch elegantly on the number of arguments that argument 'f' take. So would you instead approve of a pull request based on something like this? Or something of the sort?
|
I don't think its quite right to thing about tasks being interrupted or other tasks taking control. Tasks are not interrupted or preempted by other tasks. The only way an inactive task ever gets to run is if an active task calls So, if you have a task that never calls The key thing to realise is that the task "scheduler" is not an all-powerful omnipresent entity that lives in a parallel dimension (a thread scheduler is like that). The task "scheduler" is just an ordinary Julia function that has to be explicitly called by another Julia function. The "scheduler" is the function wait()
while true
if isempty(Workqueue)
c = process_events(true)
if c == 0 && eventloop() != C_NULL && isempty(Workqueue)
# if there are no active handles and no runnable tasks, just
# wait for signals.
pause()
end
else
reftask = poptask()
if reftask !== nothing
result = try_yieldto(ensure_rescheduled, reftask)
process_events(false)
# return when we come out of the queue
return result
end
end
end
# unreachable
end |
You could add a header(ws::WebSocket, k, d="") = header(ws.request, k, d)
hasheader(ws::WebSocket, k) = hasheader(ws.request, k) That would give the |
Re dispatching based on different function signatures in a function handle_stream(f::Function, http::Stream)
try
if applicable(f, http)
f(http)
else
handle_request(f, http)
end
catch e
...
end
closeread(http)
closewrite(http)
return
end |
f(ws)
f(ws, http)
f(ws, http, binary)
f(ws, subprotocol) I have some ideas to make this clean for users. Will try some things out and report back 😊 |
I think we are close to having WebSockets.jl in shape to work as a server with both HTTP and HttpServer and as a client with HTTP. Once we have a more complete / robust set of tests, then I would suggest comparing / contrasting WebSockets.WebSocket and HTTP.WebSockets.WebSocket more deeply and consider revising the WebSocket type accordingly. For example, the String bytes issue together with the expected performance enhancements, I am liking @samoconnor's My wish is that a single WebSockets module will emerge from this that we're all happy with 🙏 |
Sam, thank you for taking the time to correct my bad! It's so easy to construct wrong explanations, and hard to get out of it without help. The example below shows you're right and also shows me how I can debug some other code. It also demonstrates some seeming randomness in Julia's behaviour that I saw in the initial browser tests. This has to do with compiling, and starts to produce consistent output on the third run of the begin-end block. The first run may not print anything at all.
So for consistent message reception, perhaps good practice is simply sending a couple of echo messages at each application start. And absolutely avoid @schedule and @async in loops. Increase CLOSEAFTER if you dare. |
Eric, I'm sure you are right. With a small exception perhaps. Some of the symbols used for logging status in HTTP are not displayable in Windows. They work fine in cygwin, but are not covered by the Dejavu Sans Mono, which is the best we have in Atom, REPL or VSCode editors. You are wisely not choosing now at once between the two suggested approaches: a) Extend Websocket, b) Extend the function (s). I implemented the function approach with Sam's suggestion by overwriting the update function. chat_explore.jl The example is now serving chat_explore.html on BOTH dependencies. HTTP fans can open localhost 8080, and the old guys can open 8000. Currently, I'm trying to share the websocket handler and get this message, which I can't interpret today.
|
Menlo is a good programming font: https://github.com/hbin/top-programming-fonts |
In your ff() gg() example you are doing console IO in the async code (println), this will lead to unpredictable task switching. |
Sam, thanks for the font tip. I like the log style where you use symbols like skulls and chains. That comment about the font wasnt a good one. If it works out, lets put the font tip in readme.md and define similar show methods. |
Installed the font, Sam. It's nice but those symbols still don't display in other than the mintty cygwin terminal. Example below. Also updated the chat example. It's closing after one message, will fix. But it is now working for exploring types so we can find a good common interface. Old-style websockets pass a HttpCommon.request to the gatekeeper function. The useful part is its request.headers field, which is a Dict{String, String} currently. For HTTP origin websockets, the http.message.headers field ought to be sufficient info. The current example in chat_explore.jl doesn't make a Dict out of it. The gatekeeper function can't do the subprotocol upgrade. This would be an easy addition to add to WebSockets/HTTP.jl, I think.
|
True. Minor issue, but the cool symbols do no render in Windows 😊 |
Just an update / status.
Base.show methods are still not added. How much time this takes to finish depends on the local weather here. Eric, do you expect to make more contributions here? |
Hi @hustf, As far as I'm concerned, I'm happy with the current state of things. I expect some private packages might break, but active packages in METADATA seem unaffected. I like the fact WebSockets now has almost no real dependencies so others can extend it easily. I still think we need a shootout between WebSockets.jl and HTTP.WebSockets.jl which will likely lead to more changes to WebSockets.jl as the two converge, e.g. we may want to add payload vectors to eliminate the need for allocation, but that is second order and can probably done later. Personally, I don't expect to make any significant changes any time soon 😊 |
Hi @hustf, @EricForgy, I have some questions that maybe you can answer given your experience using WebSockets with real live browser and js framework implementations...
WebSockets.jl/src/WebSockets.jl Lines 348 to 367 in bafb175
|
Looking fwd. to reply. In the meantime: "we may want to add payload vectors to eliminate the need for allocation". Logging buffered amount? Access to txpayload rxpayload? Please elaborate. Also, what have you found in the thinking box about which info the user might need to modulate streaming speed? |
Sam, We'd like to support somebody using Julia to get rich on second-to-second trading, or somebody making RC cars that send intermittent bursts of sensor data. Data must be buffered on a frame-to-frame basis. The sensors / PLCs wouldn't know the length of the message when they start sending. Chrome obscures the difference between message and frame (check F12 / network / ws - there's no distinction). The javascript user don't get a chance to decide. Perhaps the browser adapts based on the current network? So extended logs from our local browser tests wouldn't give a definite answer. Regarding binary, there's an example in the tests. Javascript doesn't get access to the binary array. It can make views into it, but it's job is really to just pass a reference to the lower-level part of the browser that actually does the job of displaying the picture (in Internet Explorer, we do process it). It's hard for us now to imagine how this, and the evolving subprotocol standards, can be applied. The libraries/ subprotocols that I read up on tries to hide this kind of detail from the front-end-developer ('install our library on the server, try with our server first!'). Is there any benefit for us in removing binary support? |
You should try to get invited to better parties 😂
Not that I know of, it's more a matter of learning what are the most common real-world use cases in order to be able to reason about performance tradeoffs. It seems like there is a choice with messages and frames.
If we take option 2. and basically just ignore the concept of messages, there are some performance benefits (less state to deal with, don't have to buffer-up fragments before returning result). This is an issue that only effects the receiving of data, and given that the main use case is Julia server receiving data from chrome/safari/ie, the questions become:
Perhaps the answer is to provide |
Just two cents before heading into a day-long meeting... WebSockets are for "web" (obviously 😃 ), so for WebSockets clients, I would look at the JavaScript API and for WebSockets servers, I would look at the Node API (which I would think is essentially the same?). We should not be too far off from the JS implementations I would think. Edit:
This sounds ideal to me too. |
WebSockets were intended for "web", but WebSockets for Julia just could be the optimal choice for controlling hardware or calculation-heavy back-end. It's hard to beat node.js for the general web user. Zmq defines some non-exported functions that you start diving into when meeting buffering problems. It would be nice to support API 2, but since it's clearly for highly optimized applications I suggest we only export message-based methods. |
Just in case you guys see a solution: Would it be worth to optimize this little function for the logger? It's used in conjuction with a truncating function to show a little of the content in a Response. I don't really know when RegExp's are faster.
|
Just to let you know things are coming along. I'm working on adapting the specialized logger, which is quite time consuming. With the current pace, it may take yet another month before merging this branch, but again, dependent on local weather. |
I am not concerned at all, but I am confused why a logger would hold up this PR. Isn't it two completely separate things? 😊 |
I admit to being in doubt here too, but I think we need to be thorough this time. A logger is already implemented in the test suite, but it uses globals for redirection of output (which allocates some memory at each call and affects speed for small messages). Websockets is a very enabling technology so I hope we can reduce the effort for new users, and also make the test suite easier to maintain by including the improved logger with show methods and @requires dependencies into the main code. We also introduce a new and very enabling feature, but I am loath to go out and proudly recommend dropping zmq without having more examples. Search discourse zmq for recent examples. Should we add links to eg Pages.jl? |
Comparing ZMQ to the revamped WebSockets: ZMQ has plenty speed, but on Windows also a lot of latency unless we yield() to it several times per message. It might do better in a threaded test (?). For user interfaces, latency and warm-up is really critical, so this branch looks great so far for tying together GUIs with backends and with little code. Small bed time story: I imported WebSockets.open, then modified 'open' in order to include optionalProtocol. Around two pages up from the bottom of the stack trace I find:
...so reading a text file ends up in trying to open a HTTP connection... Just another story about the wrong path. The 'open' methods are perhaps a little too generic, although they are not normally exported. '_open' might be better style? On another note, user code could be clearer and shorter if we added more information into the WebSocket type rather than calling multiple methods (like in the current chat_explore.jl upgrade function). If we do, we can more often re-use functions. This information:
If this info could be used in dispatching, it would make for elegant code, depending on preferences. But T in WebSocket{T} is already used to distinguish between socket types so I guess we don't dare touch that. So a couple of more fieldnames to WebSocket is preferrable. What were the disadvantages that led you to take away the .id field? Types in Julia usually have very few fields. Why is that? |
Importing module MyModule
HTTP.WebSockets.open(f::Function, x::Dict) = HTTP.WebSockets.open(f, x["url"])
end
It just smelled bad. It suggested unneeded coupling, poor separation of concerns, or leaky abstraction layering. Wikipedia has some decent summaries of basic software engineering principles: |
Thanks for the reply, Sam. I appreciate those hand-picked links. Having read up on words, I may use some of them against you in future discussions! You are right about importing HTTP.WebSockets.open into a module which relies on 'open' would be bad. But I didn't. My module relied on 'readdlm' with a file name as an argument. Which is accessible from any module. This type of errors used to occur much more often with Julia a couple of versions ago. People are getting more used to the code style which makes Julia actually work. I don't know what is the best code style for making Julia actually work. One idea has been to export the functions expected to be used outside. The best documentation has sometimes been what you get when you type 'using Colors;whos(Colors)'. HTTP follows a different style than that, which is OK. Another style idea is prefixing internal functions with underscore: WebSockets._checkupgrade(..) would be an example. Thank also for the explanation about Websocket.id. Makes sense, and a bit more so after reading the wikipedia links. I'll add another reason for keeping the number of fields down. It becomes very difficult to make flexible type constructors when there's more than say four fields in a type. Long argument lists in general are a pain. |
I just pushed some changes after testing with Firefox. This is just a preliminary, working version before implementing other changes:
|
If you have defined a method of |
..but I dont see this as a bug at all. There is no reason to import HTTP.open normally. Httpserver and the old websocket avoids this little trap by defining lots of ad hoc types like httphandler and websockethandler. HTTP avoids it by not exporting, not bringing it into the user code scope. I have also not tried to reproduce. There might be other explanations, although the stack frame seems clear. If there's an actual crash with base syntax, which is always in scope of course, it is clear where the change should be made. Open is probably not moving out of base ever, although there used to be many more such functions to avoid in previous versions. |
Just pushed an update, removing some lines with temporary debug code that remained by accident. This temporary code remains in HTTP.ugrade:
I am not sure we should do any try-catch in the upgrade function at all. HTTP would catch the errors anyway. It would probably be better to play along with HTTP's error catching and logging. |
r. status provokes an error if r is is a Request and the method is not GET. From Http.jl:90:
The error is
I guess the use case for checking a Response is debugging? If that's interesting I believe this would work:
|
This was fixed in HTTP.jl, but maybe is copy / pasted here before the fix? PS: Flight was delayed. It is 3:24am and I just checked into the hotel with meetings in the morning so this is a drive by comment without looking at the code 😅 |
I hope the meeting turned out well anyway! I just pushed the correction above. |
Regarding "binary" websockets. A optional protol could choose to have every 42nd message be text if there was a reason for it, although all frames in a message should be of the same type. Eric, you actually do something like that in the rewritten binary test. You transfer the UUID as text, then in the next message you read binary data. On the browser side, the 'user', i.e. the javascript guy, needs to poke in at a slightly deeper level to speed up how incoming binary messages are interpreted. So the binaryType is "arraybuffer" or "blob". The very same websocket can happily continue to receive text messages. Ideally, javascript never accesses the contents of the binary but passes references to where they are needed. But this browser distinction does not apply to clients generated by e.g. Julia or Python. So we can choose to test for 'binary' websockets but this would rather belong in the examples folder. And the Websocket object or the upgrade function shouldn't convey information about 'binary'. That info is fully defined by the optionalProtocol. P.S. |
Just pushed some changes, still a temporary version. Modified the HTTP upgrade function to be in line with the old upgrade function:
Modified the HttpServer upgrade function:
Modifed WebSockets.jl |
Calling WebSockets.open is still making noises when not done in quite the right way. Here is another example for later checking:
|
Also, some thoughts about making the interface extendable beyond local experimenting on a computer. We don't need to do this now, but it's a good time to pick the best parts from HttpServer and HTTP. It is recommended to do some further checks of the upgrade request beyond what 'upgrade' does now. For local experimenting, this can of course be neglected, so the user shouldn't have to bother about it.
So one way to tell 'upgrade' which connections to accept might be requiring a WebSockethandler object with a dictionary of acceptance criteria, or just fields with liberal starting value. This is more like the stricter interface of HttpServer. Loosely, a WebSockethandler could be defined with: For reference, these are typical headers from Chrome: |
I experimented a bit locally with changing the @requires, because they lead to more recompiling. But the @requires is the only way I get Atom to work. It seems some parts of Atom use HTTP and some parts use HttpServer. Here's a graph layout of the dependencies neighborhood from a couple of months back. This includes testing dependencies. The lighter ones are the dependencies on this package. In change_dependencies branch: |
Just pushed a commit with added benchmarks and logging utilities. See commit message. The other significant change is adding a non-blocking read with timeout during closing handshakes. This hopefully gets rid of most ECONNRESETS. There will be no large benefit to merging and tagging this while on Julia 0.6, since it adds seconds of loading time to e.g. Atom. That is less of a worry if a faster alternative is ready for 0.7. |
Just pushed commit 8edb459, which reverts a major change from master. Control frames, like PONG, now iterates to the next control frame instead of returning. The commit also includes ServerWS, serve(::ServeWS, etc...) and some constructors. |
Cool to see progress 👍 I doubt HTTP.Serve will be extended. In fact, I believe removing it was discussed and is likely in favor of "listen". |
Commit bb9af69 reinstates readframe(ws) checking for mask.
The original test was probably removed to enable WebSockets|client, but since the function now takes a WebSocket and not just any IO, we can do this. |
It would be interesting to know how the Serve discussion turns out. I added this interface to try and get rid of console output from HTTP, but was only able to turn off some messages. 'ServeWS' assumes that user understands http requests and responses, but doesn't know what streams are. This is nice for some. But then it demands that users know about these ad-hoc function wrappers which don't really add any functionality: HttpHandle, WebSocketHandle. They do add some structure to defining servers, though. This structure would read clearer using keyword arguments. Using a specific type for these function wrappers is also beneficial for logging dispatch. I especially don't like the term WebsocketHandler. A handler is a function that reacts to an outside event and returns something useful. A WebSocketHandler doesn't do that. I would say it's an event handling subroutine that does various stuff with the argument. But then, I'm not a computer guy. If you can think of alternative names or interfaces, I would appreciate the input. |
Commit 15adbdf introduces status codes for closing handshakes, server and client side. |
Commit ac86833 brings test coverage to ~93%, before pulling in browsertests. Julia 0.7 alpha is out, so it's time to merge this branch very soon. |
I thought it was merged already :) |
One would, looking at the dates. Blame the weather! I'll have a look at the documents before merging. We're at 100% compliance with the standard I hope, except for checking that no-one used the reserved bits. |
Pretty much a large reshuffle of Readme.md. |
Commit ada7f2d reshuffles Readme.md again to make the actual interfaces / starting point clear. Also improved inline docs for WebSockets.serve. Remaining: Restructure browsertest. Include the 'client' example from readme in examples. Update wiki. But the wiki is edited in the repo web interface, and interface is settled, so I'm merging this to master now. |
Closing this long side issue. |
Due to the complexity of supporting both HTTP and HttpServer, we liberally merge PRs to this branch for easier experimentation. There's also issue #84, but that doesn't need to be cluttered with 'caps and tabs' details.
'Code review' and TO DO comments go here.
I'm working on a file-by-file list, but this is the most core. On branches HTTP/master and Websockets/Change_dependencies.
The text was updated successfully, but these errors were encountered: