Support for socket.send and intercept callback

noway commented 7 years ago

That's about it. There's also __hash__ function added to Peer for convinience and Host.service function is fixed to be in accordance to Host.check_events

aresch commented 7 years ago

Would it be possible to add tests for the new code?

noway commented 7 years ago

Don't have them on hand, but i'll try to come up with it in several days.

noway commented 7 years ago

Finally got existing tests working, there was a problem with deallocation and the fact that global variable kept the reference and did not allow gc to clear a Host instance. Weak references to the rescue.

noway commented 7 years ago

added the behaviour of those 2 functions into test_server.py and test_client.py. It makes sense to keep them there because intercept_callback is by design made for something event loop based. Those 2 tests provide loop based approach.
added 1 unit test for both of them in test_unit.py too
fixed a couple of bugs along the way
all is working and it looks good to me. Would be happy to hear out if you have any concerns.

@aresch

noway commented 7 years ago

Splitted up tests more, so they are more like new tests, instead of modifying existing ones , and fixed those small things from the feedback.

Now unit test waits for connection in a loop and only then starts interception requests too.

I've tried python 3, but run into problems with weak references. i'll definetely will look into it, but is there a hard requirement of py3k compatibility?

aresch commented 7 years ago

Yes, Python 3 is a hard requirement. Python 2 is deprecated and should no longer be used.

noway commented 7 years ago

I'll look into py3k soon-ish, the problem was weak references. At piqueserver, py3k is WIP too so it's important.

noway commented 7 years ago

For some reason, test_server.py doesn't get intercept_callbacks. py2k does it perfectly, while py3k test_server is silent

dunno why's that happening really...

noway commented 7 years ago

alright, figured it out, it was because "SHUTDOWN" == b"SHUTDOWN" was there

Pretty much it's all done. Works both py2k and py3k.

The only thing is that peer.disconnect() and peer send packet SHUTDOWN need better treatment (in the tests). Need to think about how to run them from receive_callback so that it sends SHUTDOWN first, then gets acknowledgement, only then disconnects while doing it all in a receive_callback. Just trying to reuse the testing paradigm you are using where loop does event = host.service(1000) on each iteration, while incorporating that kinda out-of-the-loop receive_callback

Meanwhile you can have a go at another review iteration :-)

noway commented 7 years ago

Sorted out the tests. Made the disconnect logic for intercept_callback test really neat: disconnect is scheduled in the callback and then performed in the main loop.

While looking at it, I found problem that the tests are non-deterministic. First, I thought that tests might be failing sometimes for me because of different random strings. So I changed os.urandom to the seedable random.randint so that tests can be run deterministically (usage: python test_server.py & python test_client.py 42 where 42 is the seed)

Turns out it's all good with the random strings, but the problem was

peer.send(0, enet.Packet(msg))
host.service(0)
peer.disconnect()

Changed host.service(0) to host.service(100) to avoid the problem. The issue was subtle: python test_server.py wouldn't stop sometimes because it might not receive SHUTDOWN message. This change fixes it.

FYI: this is the second change I did for the existing tests. First one was changing "SHUTDOWN" to b"SHUTDOWN" (introduced problems to py3k runs)

To my best knowledge, this PR is ready for the codebase and it indeed looks good to me.

aresch commented 7 years ago

Thanks for all the work addressing my comments!

aresch / pyenet

Support for socket.send and intercept callback #4