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

Socket Support #550

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@JasonTheKitten

JasonTheKitten commented Jun 14, 2018

Adds socket support.

Here is an example script:

local host, port = socket.lookup("https://google.com/")
local sock = socket.open(host, port, true)
sock.write("GET /search?q=Google HTTP/1.1\n\n")
print(sock.readAll())

Also supports ASYNC requests

There is a small minor bug with sock.length and sock.clear, where Java seems to divide the output into buffers, but it does not matter too much.

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 14, 2018

I think websockets (as implemented by #395 or CC-Tweaked) make more sense, being easier to use, having better security (wss out of box) and still being powerful for real-time communication.

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jun 14, 2018

To echo ape's sentiments: in the general case, it's probably easier to use websockets. They're a lot less finicky and end up being a lot more "event driven", which fits well with CC's design.

That being said, I don't think there's necessarily a problem with having TCP support. However, there are a couple of potential issues I see with this PR.

  • A rather nitpicky one, but please turn on whitespace visualisation and run your code through a source formatter. There's a really weird mix of tabs and spaces in here, which makes it incredibly hard to read your code.

  • AsyncAction ends up spawning a thread for every single task. While CC is does this a lot already (#218), we really should at least look into pooling these. That being said, it's probably going to be better to use the non-blocking versions of the sockets. This means you can have one thread which runs select on all sockets, passing off events to the relevant computers.

  • We need to discuss how information should be encoded. Your write method uses the default system encoding (which may or may not be UTF8) and the read method seems to return the codepoints as integers concatenated together (so .read(2) on AA will return "6565").

  • .length doesn't really make sense as a function TBH, I'd just remove it. None of the other handle-like objects have it, so I don't think we loose anything.

  • I don't really see the use of .getCertificates.

I'd personally like to see something like CCTweaks' (or OC's) implementation, though I'm obviously a little biased.

One of the things I like the most about #395 is that it's entirely event driven and, while it's not really possible to achieve that with TCP, it would be nice to have something which doesn't require quite the same level of polling.

Correct the docs for sockets
Some of the info was for an older implementation of sockets I wrote
@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 14, 2018

.getCertificates is so that a program can know or notify the user how secure the connection is
Perhaps I could remove .length and .clear, because they are buggy anyways, but I feel it would be best left off to be removed on the lua side and not removed completely from the java side

As far as .read goes, it is probably best to just repeatedly call the function, because it does not pad the numbers, so I should probably remove that argument...

I've always had the problems with the tabs and space, I used to never used tabs because of this, but I recently started using them...

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 14, 2018

I think websockets (as implemented by #395 or CC-Tweaked) make more sense, being easier to use, having better security (wss out of box) and still being powerful for real-time communication.

I didn't like how CCTweaked didn't let you make your own requests... One of the reasons I wrote this
Because I don't think many sites support websockets (Tried them on google, did not work), but most site support TCP sockets

I've been wanting a raw socket implementation for two years now, and, well, here it is

Plus, while I haven't tried it, I believe you can emulate websockets with TCP sockets...
https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_a_WebSocket_server_in_Java

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 14, 2018

I was thinking of writing a higher-level api for this that will also automatically process the incoming data for you and re-process it when new data is received, using a handle similar to the HTTP handles, but with a write function

So you could do something like this:
local wsock = wrappedsocket.open("https://google.com/", nil, true)
wsock.write(wrappedsocket.formatR("/search?q=google","GET", {}))
sleep(5)
wsock.reprocess()
print(wsock.readAll(), wsock.getResponseCode())
wsock.close()

Does this sound worthwhile?

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jun 14, 2018

I didn't like how CCTweaked didn't let you make your own requests... One of the reasons I wrote this Because I don't think many sites support websockets (Tried them on google, did not work), but most site support TCP sockets

I suspect we're thinking of using TCP sockets for different things. From what I can tell, you're trying to emulate HTTP requests, which seems rather pointless as we have a rather decent library for that already.

TCP sockets (and websockets) are far more useful when you want to keep connections open for a long period. This means you don't have to poll a website for information - it's streamed directly to you. A great example of this is Krist - you can connect via websocket to the API and it'll send you a message every time there's a transaction.

The key point here is that (TCP|web)sockets aren't useful for connecting to arbitrary websites, they're useful for connecting to specific services. Be it the krist API or an IRC client.

.getCertificates is so that a program can know or notify the user how secure the connection is

I guess I'm not seeing a use-case for this. If you're connecting to a server, the certificate is going to already be signed by a CA, and so should be pretty secure. Sure, one's web browser exposes this information, but how often do you actually check it?

As far as .read goes, it is probably best to just repeatedly call the function, because it does not pad the numbers, so I should probably remove that argument...

The old binary file handles used to do this, and it was rather painful to use (not to mention insanely inefficient here as you're spinning up a thread for each call). It shouldn't be especially hard to add this - you've already got an input stream so you can just yoink the logic from the existing file handle code.


I don't think having sockets is a bad idea, I just think we all need to sit down and brainstorm what they'd be used for and how we want them to work before writing any code. It's much harder to go back and fix things once we've added them, as you run the risk of breaking backwards compatibility (cough #183 cough).

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 14, 2018

Sockets can be used to communicate with a server that uses a constant input stream. In addition, we can emulate websockets

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 15, 2018

Websockets can also be used for constant communication, and while re-implementing websockets using a TCP layer is technically possible, it would result in a variety of issues:

  • LuaJ is already slow, implementing a high-level protocol like websockets (which are themselves built on top of HTTP) is only going to make it worse
  • Security pretty much goes out the window, unless you want to implement modern TLS yourself and then copy all the necessary certificates for HTTPS (and by extension, WSS) to the computers
  • While we're on the topic of reimplementing things, you'd also need to either implement or expose DNS, or use an external service to resolve domains

I could go on and on...

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jun 15, 2018

Guys, this shouldn't boil down to some "TCP vs Websockets" debate, as I don't see a reason we can't have both. One is easier to use, one is more powerful.

I think what really needs to be discussed, is how we want people to use it, and thus how we want the API to look. One of the complications that TCP has is there is no distinct idea of a packet or message, so we can't really do a rednet-esque implementation like #395 does. I think the best solution is something akin to what this PR does, albeit with a few more events thrown in:

A potential socket API, put in a spoiler as it's rather large

socket library

socket.connect(host:string, port:number[, secure:boolean]):table|nil,string

Creates a connection to a given host/port combination (note host can be a domain name or IP address). Behind the scenes, this will delegate to some async method which queues a socket_success or socket_failure once connection succeeds/fails.

Socket handles

handle.read([len:number]):string

Read at most n bytes from the socket or, if no n is specified, as many bytes as are available.

handle.write(msg:string)

Write msg to the socket (as a set of bytes).

Issue: What happens when not all of msg can be sent for whatever reason? How do we signal this to the user, and how can we recommend they try again.

handle.close()

'nough said

Event socket_readable: host:string, port:number

Fired when incoming data arrives, denoting there is information which can be read from the socket.

**Issue: ** What happens if you've multiple connections to the same host/port combination? Should we give each socket some unique identifier instead?

Event socket_closed: host:string, port:number

Fired when the socket is closed. I'm less sure/fussed about this one.


This was knocked up pretty early in the morning, so there's definitely some issues, but I think it's a good starting point. Obviously it's possible to add additional methods to socket handles, but it's a nice skeleton.

That being said, I do welcome radically different proposals. While I think an "event driven system" is necessary, I do think I run the risk of just re-inventing CCTweaks :/.


It might also be worth moving this conversation to some "TCP sockets" suggestion issue ¯\_(ツ)_/¯.

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 15, 2018

I was specifically responding to the idea of re-implementing websockets using a lower level API. Having both is fine, as long as they're implemented in a sane way.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 16, 2018

I do not understand how you could have just one thread with non-blocking.

I tried out a single thread but found that that meant you have to wait for one task to finish before you perform the next action, which is especially troublesome to debug if you accidentally enter an infinite loop...

Here is the new code I was trying out:

-snipped-

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 17, 2018

There are already many applicable concurrency features in the Java standard library (i.e. Future, Executor, and various locks/latches), at least use those.

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jun 17, 2018

I do not understand how you could have just one thread with non-blocking.

The distinction between blocking and non-blocking IO is pretty simple:

Obviously this is fantastic, as the .read method can be rather simple - there's no need to faff around with AsyncActions as the whole thing isn't going to block!

Now, the obvious issue here is that people no longer know when to call .read - you can't call it straight away and wait for something, as it'll just return. Enter java.nio.channels.Selector: you register all your sockets in one selector, and call selector.select(). This will wait (or block) until one (or more sockets) are readable1 . You now just run this in a loop on a separate thread and fire off events to CC where appropriate.

It's then possible to build some sort blocking read method on top of this - it tries to read from the socket, pulling socket_readable (or whatever) events when it doesn't get anything. However, as this is just using CC's events (and thus coroutines) instead of actual blocking operations, other Lua code can run at the same time.

1. You can also listen to when a socket is closed, opened and writeable.
@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 17, 2018

Why do we need all this complicated stuff, why can't we just create threads and then delete them when done, like I originally did, before I modified it to only use a single thread...

I wrote the AsyncAction class because I originally did not include async methods in my initial code, but changed my mind, and I didn't want to have to deal with all this complicated stuff that will turn my code into spaghetti.

@Lignum

This comment has been minimized.

Contributor

Lignum commented Jun 17, 2018

Why do we need all this complicated stuff, why can't we just create threads and then delete them when done, like I originally did, before I modified it to only use a single thread...

Because starting threads is expensive, and can be abused by malicious players, unless you have a thread pool, which we don't.

[..], and I didn't want to have to deal with all this complicated stuff that will turn my code into spaghetti.

If modeled properly, it won't turn your code into spaghetti. That applies to basically everything.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 17, 2018

This is mostly ready, just that one thing about the threads...

**EDIT: **making a quick edit

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

OK, GitHub should recognize my latest commit when I reopen this, fixed thread system and socket.lookup

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

The point of using Future and other concurrency features is to avoid implementing your own equivalents (AsyncAction and co). FutureTask should be used for this, and can be submitted directly to an Executor.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

The point of using Future and other concurrency features is to avoid implementing your own equivalents (AsyncAction and co). FutureTask should be used for this, and can be submitted directly to an Executor.

This is easier. I already had my thingy set up to return the values in the method, so it was easiest to do this than create extra methods. Also, if enough functions were to use AsyncAction, it would eventually save us some code.

@zardyh

This comment has been minimized.

zardyh commented Jun 18, 2018

If enough functions were to use Future, it'd save us all of that code and then all of the code for AsyncAction!

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

Your AsyncAction class is nothing but a wrapper for a ThreadPoolExecutor anyways, and if you actually used the features provided by FutureTask, it would be completely unnecessary. Counter is nothing more than a gimped AtomicInteger, minus the thread safety (speaking of which, almost none of your code is thread-safe). Your implementation right now will create a new ThreadPoolExecutor for every single computer, each capable of spawning many threads, which as previously mentioned, is quite expensive and abusable. This is a very bad™ idea.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

Only up to 2048 threads per computer. That was intentional

What do you mean gimped? it was not made with an image editor.

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

Only up to 2048 threads per computer. That was intentional

That has to be the most absurd statement I've ever heard. 2048 threads is enough to completely cripple a (real) computer, and that's before you start placing more of them down. There should be at most only a couple threads used for this, globally, and with minimum priority.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

The HTTP API uses Integer.MAX_VALUE for it's thread count. This is much lower.

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

What do you mean gimped? it was not made with an image editor.

Crippled, injured; damaged as to awkwardly impede function

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

The HTTP API uses Integer.MAX_VALUE for it's thread count. This is much lower.

That's a different issue that should also be addressed, but at the very least it uses one ThreadPoolExecutor instead of one for every single computer.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

Are you saying that I should revert to 1 thread?
AsyncAction.java (Older)

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

No, I'm saying you should properly make use of ThreadPoolExecutor and FutureTask: One shared instance of the executor, sane thread limits (not 2048), using methods provided by FutureTask rather than a weird wrapper system, and ditch AsyncAction entirely.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

Today's Windows 8.1 cpu's are fast. They should easily be able to handle 2048 very very small threads

Also I already wrote all the code. It might take a week to switch systems, and the current AsyncWrapper system allows you too easily plop new methods into the API, instead of having to deal with Future's and all

I really don't see what you have against AsyncWrapper. I thought it was better than futures, and even thought about changing the whole HTTP API to use (a modified version of) them instead of futures

Plus you don't have to have like 50 event names, it's just one async_socket event, which can be easily distinqushed

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 18, 2018

I really don't see what you have against AsyncWrapper. I thought it was better than futures, and even thought about changing the whole HTTP API to use (a modified version of) them instead of futures

It's an unnecessary layer of abstraction, considering CompletableFuture and Executor provide all the necessary facilities.

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jun 18, 2018

The HTTP API uses Integer.MAX_VALUE for it's thread count. This is much lower.

Yeah, I'm lazy and didn't want to choose a good maximum. Sorry. I honestly don't think the thread limit is a stopping factor, as both the HTTP, websocket and socket functionality is highly abuseable in other ways.

Are you saying that I should revert to 1 thread?

ape has rather lead you up the wrong alley here. It's not that what he's said is incorrect, it's just that it's the correct solution to an entirely different problem. The problem with having one thread for all of them (or 10, or 100) is that reading from one socket will block all the others. The only solution here is to have one thread per socket, which then leads onto the same sort of issues as C10k.

The solution here, as I may have mentioned before, is to look into non-blocking IO. It's possible to get this running with 4 threads across the entire system. This way you don't need to worry about resource limits, nor one socket blocking the others.

Yes, it's painful as hell to implement, but the end results are most definitely worth it.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

Ok, but quick question, does the nio class support SSL?

Hmm, GitHub is saying I cannot comment, I'll try again

@Lignum

This comment has been minimized.

Contributor

Lignum commented Jun 18, 2018

Ok, but quick question, does the nio class support SSL?

Yes.

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 18, 2018

Do whatever you want with this, feel free to fork it if you really want changes (I will be notified, may merge), I don't want to deal with this any more

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jun 19, 2018

You should probably close the PR then

@JasonTheKitten

This comment has been minimized.

JasonTheKitten commented Jun 19, 2018

He can still merge it: I'm just not maintaining it unless somebody PR's me

Anyways, I can't maintain it: My system is now missing program_files/java, and the java installer is being stuborn, despite the fact that I have rebooted many times

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment