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

Some edits (Python 2.6 compat., no websocket by default, some debugging) #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Elektordi
Copy link

No description provided.

@OliverF
Copy link
Owner

OliverF commented Jul 29, 2016

Great, thanks for this. I'll have a quick look now and do a quick test over the weekend.

@Elektordi
Copy link
Author

Hello! Did you had any time to review this?

@Silex
Copy link
Contributor

Silex commented Dec 12, 2016

I think "Prevent exception on quit" 986760e is the wrong fix.

IMHO 70501e9 from #13 is the right one.

The other commits looks fine tho, I'd just drop the last one.

@Elektordi
Copy link
Author

This seems to be different problems. In 986760e, I'm fixing a bug I think I added in previous commit:
Since you can disable the sockets, there will be exception if at the end you try to close a socket that was disabled on startup.

@Silex
Copy link
Contributor

Silex commented Dec 12, 2016

Well, ok for the requestHandler variable, but for the broadcast variable it is a mistake because it is always present. Also your fix does not take care of the quitsock. The diff should be like this:

diff --git a/relay.py b/relay.py
index e2285b3..3014132 100644
--- a/relay.py
+++ b/relay.py
@@ -24,8 +24,10 @@
 # Close threads gracefully
 #
 def quit():
	broadcast.kill = True
-	requestHandler.kill = True

- 	quitsock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
- 	quitsock.connect(("127.0.0.1", options.port))
- 	quitsock.close()
+	if requestHandler:
+		requestHandler.kill = True
+  		quitsock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+ 	 	quitsock.connect(("127.0.0.1", options.port))
+ 	 	quitsock.close()
 	sys.exit(1)
@@ -54,6 +56,9 @@ def quit():
 		logging.error("Port must be numeric")
 		op.print_help()
 		sys.exit(1)
+		
+	requestHandler = None
 
 	Status()
 	statusThread = threading.Thread(target=Status._instance.run)

Then my commit fixes the broadcast --> broadcaster typo.

@Silex
Copy link
Contributor

Silex commented Dec 12, 2016

To be honest the whole quit() code is dubious.

Instead of all these foo.kill = True, it'd be code like requestHandler.acceptsock.close(), possibly wrapped in some requestHandler.shutdown() method.

@Silex
Copy link
Contributor

Silex commented Dec 12, 2016

Here is what I have in mind as a refactoring of the shutdown process: Silex@217904d

@OliverF
Copy link
Owner

OliverF commented Dec 12, 2016

Sorry @Elektordi, I haven't forgotten about this, I've just been very busy lately!

@Silex good catch regarding quitsock. I think you're right, the whole quit process needs re-thinking. Perhaps we could open a new PR for your proposed changes and discuss there?

@Silex
Copy link
Contributor

Silex commented Dec 12, 2016

A'ight 👍

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.

3 participants