funbox / smppex

✉️ SMPP 3.4 protocol and framework implementation in Elixir
MIT License
106 stars 34 forks source link

Ranch 1.6+ compatibility #49

Closed archseer closed 6 years ago

archseer commented 6 years ago

Ranch 1.6 changed that :shoot we had to add for SSL into a :handshake. Do we simply change it and raise the ranch requirement?

https://github.com/ninenines/ranch/commit/5ada450fe10a5fc51b7f3f0900571a10547635a1#diff-cd16482a5bcdde3be9cf8e56db2bfaa2L169

https://github.com/savonarola/smppex/blob/8d82559c0db4db1ffcac2a04c04e2b5017ff4708/lib/smppex/transport_session.ex#L105

https://github.com/savonarola/smppex/blob/8d82559c0db4db1ffcac2a04c04e2b5017ff4708/lib/smppex/transport_session.ex#L144

archseer commented 6 years ago

Though I think the ESME code will use :shoot correctly, and the MC might use our patch correctly?

savonarola commented 6 years ago

Hello!

Thank you for noting this.

What should we do to handle this gracefully? I think something like the following:

1) upgrade some minor version (PATCH version?) and lock ranch to < 1.6 2) upgrade more major version(MINOR version?), make fix shoot -> handshake and lock ranch to ~> 1.6

archseer commented 6 years ago

Sounds reasonable!

savonarola commented 6 years ago

Hmmm.

Have you run into any actual malfunctions with this?

It seems that the code evolutioned in the way in which if we send shoot "manualy" (TransportSession.grant_socket), then we catch it "manually", in TransportSession.accept_ack(ref, :esme). And if Ranch sends shoot itself, we catch it with Ranch'es code in TransportSession.accept_ack(ref, :mc).

Also, the ssl test in https://github.com/savonarola/smppex/blob/master/test/integration/ssl_test.exs keeps working with Ranch 1.6+.

archseer commented 6 years ago

Sorry for the late response! That's what I meant in my followup comment, although I didn't phrase it well:

Though I think the ESME code will use :shoot correctly, and the MC might use our patch correctly?

I've deployed 2.2.8 to prod and it does work fine without the 2.2.9 changes! So I think we can revert the 2.2.9 pin, sorry for the hassle.