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

Ability to Broadcast games on the Local Area Network #3828

Open
wants to merge 12 commits into
base: develop
from

Conversation

@sladyn98
Copy link
Contributor

sladyn98 commented Jan 25, 2020

Ability to Broadcast games on the Local Area Network

Contains

JoinGameScreen.java - Added the broadcast button to be enabled only when the server is not broadcasting, along with a custom message of all the modules, ports and IpAddresse's

Broadcast.java - Added the ability to broadcast the game on all local listening servers.
The message is broadcasted every 5 minutes.
"Fixes #3760"

How to test

The broadcast button is added to the User Interface and the respective broadcasted message can be read on the Join Game Server on a machine present on the LAN.

Outstanding before merging

  • ✔️ UI button for broadcasting
  • ✔️ Make sure Broadcasting is enabled for headless server.
  • Need to add documentation for this.
…o the local area network.

JoinGameScreen.java - Added the broadcast button to be enabled only when the server is not broadcasting.
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jan 25, 2020

Can one of the admins please verify this patch?

@sladyn98 sladyn98 requested a review from pollend Jan 25, 2020
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Jan 25, 2020

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Jan 25, 2020

It's wip now?
I don't sure what using all upped non loopback interface is good idea. I haven't idea how make this part better :3
Please, add todo list to Outstanding before merging.

@sladyn98 sladyn98 changed the title Ability to Broadcast games on the Local Area Network (WIP ) Ability to Broadcast games on the Local Area Network Jan 25, 2020
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Jan 25, 2020

@DarkWeird I added the Button for the broadcast but when I run the game it does not show up , am I missing something ?

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Jan 25, 2020

I am not familiar with NUI.
Use it:
https://github.com/Terasology/TutorialNui

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Jan 26, 2020

Don't forget cleanup resources (shutdown executor on exit)
Maybe better integrate server to network subsystem, or separate.
Be careful, we have some works with nui extraction in separate lib.
I wanna see your code, working on headless server too :3

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Jan 27, 2020

Don't forget cleanup resources (shutdown executor on exit)

Yes adding it :)

Maybe better integrate server to network subsystem, or separate.

With respect to integration with integration with network subsytem, this is already separate module isn't it ?

Be careful, we have some works with nui extraction in separate lib.

Yeah it does not break anything for now, Do I need to take care of something specific ?

I wanna see your code, working on headless server too :3

Working on the headless server part,

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Jan 27, 2020

I think better implement as part of network system, or near.

Your broadcast placed at nui's package now :3

@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Jan 28, 2020

This pull request introduces 1 alert when merging 835c573 into 6671bf5 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null
sladyn98 added 2 commits Jan 30, 2020
Added a translation for the broadcast and terminate broadcast features also closed the executor thread after termination.
@sladyn98 sladyn98 changed the title (WIP ) Ability to Broadcast games on the Local Area Network Ability to Broadcast games on the Local Area Network Feb 1, 2020
@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 1, 2020

  1. Fix imports, plz (broadcastserver have import on nui)
  2. Why broadcastserver in headless setup use DI stuffs(its work?), in nui not
  3. What with multiple network interfaces?
    If you bind server on all interfaces - it is 0.0.0.0 - unreachable target port. You need broadcast every interface ip.
    If specific(one)- you not need broadcast to others network. (It's my imho. Based on network knowledge. Not deep terasology's network knowledge)
@SoniEx2

This comment has been minimized.

Copy link
Contributor

SoniEx2 commented Feb 1, 2020

maybe use UPnP or something?

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 1, 2020

@DarkWeird

  1. Why broadcastserver in headless setup use DI stuffs(its work?), in nui not

I am not sure what DI stuff, could you explain what do you mean ?

If you bind server on all interfaces - it is 0.0.0.0 - unreachable target port. You need broadcast every interface ip.

   Vector<String> container=new Vector<String>();
        container.add(message);
        String m=classname.getMYIP();
               //here u have your own IP address..now suppose your IP is 192.168.1.1.basically m sending hello packet to every other machine.if that machine is active ,it will reply n we get to know that machine is active......
               for(int i=1;i<255;i++)
           {
                        try
                               {
                     
                    String n="192.168.1."+i;//192.168.1.1,192.168.1.2 n so on.........to 192.168.1.255
                     
                        new Thread(new client(n,container)).start();//create client threads n send hello packet         to every other machine......
                        System.out.println(i);
                     
                }catch(Exception e){e.printStackTrace();}
             
        }

We could do this , it is a much brute force approach and only checks for 255 machines.
What we are doing here is basically

  • Determine your own IP address
  • Determine your own netmask
  • Scan all the addresses (except the lowest, which is your network address and the highest, which is your broadcast address)
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 1, 2020

maybe use UPnP or something?

I guess this is very susceptible to attacks, it is a bit of a dangerous approach ?

@SoniEx2

This comment has been minimized.

Copy link
Contributor

SoniEx2 commented Feb 1, 2020

properly configured UPnP isn't any more risky than UDP broadcast.

improperly configured UPnP doesn't suddenly become more dangerous because apps are using it. improperly configured UPnP is inherently dangerous, but doesn't affect the safety of terasology.

whether UPnP is properly or improperly configured depends on the router, not on terasology.

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 1, 2020

@sladyn98

  1. Dependency injection. In our case - @ In annotation.
  2. I say about several network cards (wifi/lan, several lan)
    You can start server on all network interface(use ip 0.0.0.0, as ip for bind server) or specific(192.168.1.1 (lan for example) or 10.0.1.1(wifi for example))
    If you start server on wifi, your code will broadcast invalid ip for lan(for wifi it bradcast too)
    If you start on any network card(interface), your code will broadcast invalid ip for one of network interface(lan or wifi)

Maybe, some problem will be with docker, vpn, etc. With all what generate virtual network interfaces, if it installed/used on system

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 1, 2020

@SoniEx2 i think implemention of UPnP can be as separate PR.
IIRC, UPnP configure router's routing from WAN to LAN, isn't it?

Hmm, broadcast to WAN ?

@SoniEx2

This comment has been minimized.

Copy link
Contributor

SoniEx2 commented Feb 1, 2020

Please read up on UPnP https://en.wikipedia.org/wiki/Universal_Plug_and_Play

It is a more suitable solution to this problem.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 1, 2020

Please read up on UPnP https://en.wikipedia.org/wiki/Universal_Plug_and_Play

It is a more suitable solution to this problem.

Thank you so much for the feedback, really appreciate it. Will Read up on this.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 1, 2020

@DarkWeird Looking at the dependency injection, rather I left it to the class to be responsible for the creation for the object. It would work fine, if i hard code the object creation, If you suggest that is a better approach, I am definitely on board :)

// ----------------------------------------- //
StringBuffer discoveryMessage = new StringBuffer();
discoveryMessage.append("M-SEARCH * HTTP/1.1\r\n");
discoveryMessage.append("HOST: " + ssdpIP + ":" + ssdpPort + "\r\n");

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Feb 2, 2020

Author Contributor

This looks to be sufficient in order to broadcast both the port and address.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 2, 2020

@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Feb 2, 2020

This pull request introduces 1 alert when merging 1f0cd29 into 6671bf5 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null
@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 2, 2020

Upnp Looks good.
@SoniEx2
Is it working without router?
It can work in vpn? (Hamachi e.g)

Localhost as source address looks odd. Its working?

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 2, 2020

Don't forget change System.out to logger usage

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 2, 2020

Is it working without router?

Yes it is working with router. I checked the original program with wireshark and it got the packets.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 2, 2020

It can work in vpn? (Hamachi e.g)

IIUC VPN has the purpose of connecting networks so if we want to reach another system via a VPN you will have to establish a network connection , a Java API for all of this protocols will be (nearly) impossible, since VPN is handled by OS drivers and not on the application level (where java has its place) in most cases.
Although we could detect if a vpn is up and then try and stop the broadcast since outer connection would not be possible if that is the use case we are targeting.

try {
    for( NetworkInterface intf : Collections.list(NetworkInterface.getNetworkInterfaces())) {
        // Pass over dormant interfaces
        if(!intf.isUp() || intf.getInterfaceAddresses().size() == 0)
            continue;
            if ("tun0".equals(intf.getName())){
                // The VPN is up
                break;
            } }}
@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Feb 2, 2020

This pull request introduces 1 alert when merging 6858ecc into 6671bf5 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null
@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 2, 2020

@sladyn98 using tun0 - bad idea.
Different vpn clients use different names, and can setup different names.

You don't must think about special handler for vpn(on application level they looks like common network).
I want know, your code can properly work with vpn? (Some guys want play together on self private server, but haven't providers, which open port/ip for connection from external net)

@SoniEx2

This comment has been minimized.

Copy link
Contributor

SoniEx2 commented Feb 2, 2020

please use an UPnP library. it'll also save you figuring out the IP. it'll handle all that for you.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 2, 2020

please use an UPnP library. it'll also save you figuring out the IP. it'll handle all that for you.

Appreciate the feedback, do you mean cling?

@SoniEx2

This comment has been minimized.

Copy link
Contributor

SoniEx2 commented Feb 2, 2020

I'd suggest using https://github.com/cybergarage/cybergarage-upnp because that's still maintained.

I2P uses it, so it's probably gonna be maintained for a long time.

edit: upon further inspection, I2P seems to have their own fork of it. I wonder if I can get them to publish it separately.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 2, 2020

EDIT : @SoniEx2 Yeah had a look :) We could use still use the cybergarage-upnp

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 2, 2020

@sladyn98 using tun0 - bad idea.
Different vpn clients use different names, and can setup different names.

You don't must think about special handler for vpn(on application level they looks like common network).
I want know, your code can properly work with vpn? (Some guys want play together on self private server, but haven't providers, which open port/ip for connection from external net)

Yeah this would work over private networks smoothly 👍

a corresponding broadcasting server.
This commit allows the server to respond only when it
detects a broadcast message.
At the start of the broadcast a server sends in a broadcasting
message and then goes on to listen for responses.
@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 10, 2020

  1. Remove unused channel field
  2. Move strings(request, response) to static
  3. Add explicit encoding for byte-string transformation(toBytes,new String)
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 11, 2020

  1. Add explicit encoding for byte-string transformation(toBytes,new String)

@DarkWeird Could you let me know where exactly do you prefer this encoding. The getByte Method does do the same isn't it

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 11, 2020

This methods using system default encoding (windows-125* on windows, utf-8 on linux(or any other encoding, if user has very interesting ideas :) )).
One string with different encodings can represent as different bytes.

Further, it has not zero chance to use different, non equals encoding.

Oh, java encoding called as charset.

My(and many libraries authors) recommendation: explicitly use UTF-8 ( new String(bytes, charset), getBytes(charset))

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 12, 2020

@DarkWeird I added the encoding. Thanks

private DatagramSocket getReceiveSocket() {
try {
if (receiveSocket == null) {
receiveSocket = new DatagramSocket(8002,
InetAddress.getByName("0.0.0.0")); // 0.0.0.0 for listen to all ips
receiveSocket.setBroadcast(true);
}
} catch (Exception e) {
logger.error("Broadcast Exception Encountered" + e.getMessage());
}
return receiveSocket;
}

private DatagramSocket getSendSocket() {
try {
if (sendSocket == null) {
sendSocket = new DatagramSocket(8001, InetAddress.getLocalHost());
sendSocket.setBroadcast(true);
}
} catch (Exception e) {
logger.error("Broadcast Exception Encountered" + e.getMessage());
}
return sendSocket;
}
Comment on lines +93 to +116

This comment has been minimized.

Copy link
@DarkWeird

DarkWeird Feb 12, 2020

Contributor

In these methods:
If sockets cannot to create(e.g. ports already binded), you will receive NullPointerException, because methods will return null at the end.

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Feb 13, 2020

Author Contributor

We won't be needing a new socket right? Because we would be using the same one to send the request. Do you think there is a better way to do this?

This comment has been minimized.

Copy link
@DarkWeird

DarkWeird Feb 13, 2020

Contributor

I'm would do separate socket intialization and getters, because it can throw exception.
If it will throw exception, then broadcast feature cannot will work properly at least.
If you will use one class instance - you use one socket.
Additional, You can split client and server functionality

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Feb 25, 2020

Author Contributor

Thanks for the feedback but in my opinion this code should catch broadcast errors if the sockets are null or the ports are unavailable so that the broadcast can be terminated immediately. I do not think we need more modularity in terms of client and server functionality because this offers us the best error handling.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 25, 2020

@DarkWierd got most of the changes committed :)

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 26, 2020

@sladyn98
Your Runnable runnable with listenToBroadCast not using now.

I don't think what PR done.
You print response in log. How is regular player can use your feature? Regular player don't use console.

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Feb 26, 2020

@DarkWeird

You print response in log. How is regular player can use your feature? Regular player don't use console.

Yeah this was something I was waiting for feedback on. Do we want to build a separate area for displaying the servers or just show up on the JoinGameScreen.?

private static DatagramSocket receiveSocket;
private static DatagramSocket sendSocket;
private static boolean turnOnBroadcast;
private static final Logger logger = LoggerFactory.getLogger(BroadCastServer.class);
private static ScheduledExecutorService service;
private static final String DISCOVERY_REQUEST = "DISCOVERY_REQUEST";
private static final String DISCOVERY_RESPONSE = "DISCOVERY_RESPONSE";
Comment on lines +29 to +35

This comment has been minimized.

Copy link
@DarkWeird

DarkWeird Feb 26, 2020

Contributor

Make fields non-static, except DISCOVERY_REQUEST/RESPONCE and logger.

Executors.newSingleThreadExecutor().execute(new Runnable() {
@Override
public void run() {
while (true) {
try {
while (turnOnBroadcast) {
listenToBroadCast();
}
} catch (Exception e) {
logger.error("Broadcasting has been interrupted" + e.getMessage());
}
}
}
});
Comment on lines +45 to +58

This comment has been minimized.

Copy link
@DarkWeird

DarkWeird Feb 26, 2020

Contributor
  1. Every invoke startBroadcast() will produce new unclosable Thread within newSingleThreadExecutor
    With '2'(see next) you can eat all your CPU(or problem with shared access to listening socket), if you will click many times on button.
  2. Using while (true) - bad idea. It's Busy waiting (I googled it now. it said - anti-pattern :) )
  3. You don't close sockets, listening socket will be listen while it not recieve packet or program exited, even your turn broadcast off.

I see two ways to fix it all:

  1. working with something like UDPThread(extends Thread) with closing socket on interrupt how there
    create and interrupt it on start and stop broadcast
  2. not create new executor every time
    instead while (true) use one of java.util.concurrent.* classes or Object#wait and Object#notify primitives
    setting timeouts for listening (avoid problem in '3')
private DatagramSocket getSendSocket() {
try {
if (sendSocket == null) {
sendSocket = new DatagramSocket(8001, InetAddress.getLocalHost());

This comment has been minimized.

Copy link
@DarkWeird

DarkWeird Feb 26, 2020

Contributor

Seems, it is not working in LAN. Try test this PR on several computers.

@DarkWeird

This comment has been minimized.

Copy link
Contributor

DarkWeird commented Feb 26, 2020

@DarkWeird

You print response in log. How is regular player can use your feature? Regular player don't use console.

Yeah this was something I was waiting for feedback on. Do we want to build a separate area for displaying the servers or just show up on the JoinGameScreen.?

Better separate area with something like 'LAN' title. Or something like separate Tab (if it is possible).
When I saw your PR I thought about Counter-Strike 1.6's 'LAN Games' :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.