carlosCharz / fcmxmppserverv2

XMPP Connection Server for FCM using the latest version of the Smack library (4.3.4) + Connection Draining Implementation
47 stars 33 forks source link

Added Exponential backoff to sendPacket #19

Closed TheTurkeyDev closed 6 years ago

TheTurkeyDev commented 6 years ago

Quick implementation I saw while hooking my application to the project. Let me know if you have any formatting preferences or better yet a formatter that you use for your IDE.

carlosCharz commented 6 years ago

@Turkey2349 I reused my backoff implementation that is much simpler. I don't think creating a thread and using synchronized (danger) is the best option!! Every time you receive a packet to process, it creates a thread in the app inherently. Check out this implementation + tests. https://github.com/carlosCharz/ExponentialBackOff

I think I should add the clean exponential backoff that I have because I already tested in my project but I have not push it here yet.

carlosCharz commented 6 years ago

So I mean. Your code might work but I do not think it is the best option. About the formatter I am using a custom formatter based on eclipse default one.

TheTurkeyDev commented 6 years ago

My only gripe with your current solution is that it is going to sleep the main thread. Ideally the main thread shouldn't be held up by this (or at all really), which is why I went with my strategy. I agree it isn't pretty and probably not the most optimal , but I know it works. Mind sending me your formatter? I have a habit of hitting ctr-shift-f a lot and it's messing me up not having it format to how everything else is.

carlosCharz commented 6 years ago
  1. Can you explain me more in detail your gripe regarding the main thread sleeping?

  2. My current solution with back off strategy: public void sendPacket(String jsonRequest) { final Stanza packet = new FcmPacketExtension(jsonRequest).toPacket(); final BackOffStrategy backoff = new BackOffStrategy(); while (backoff.shouldRetry()) { try { connection.sendStanza(packet); backoff.doNotRetry(); } catch (NotConnectedException | InterruptedException e) { logger.error("The packet could not be sent due to a connection problem"); backoff.errorOccured(); } } }

  3. Sure. I can send it to you but I have not finished yet my formatter. If you press that it might change some parts yet. But anyway, give me you email to forward it to you.

TheTurkeyDev commented 6 years ago

Ignore what I said about the main thread sleeping, I think I was a little quick to speak there. I guess the better question to ask is if I call sendPacket(), what is preventing my code that called the method from being held up by the Thread.sleep() that you use when the send fails. Does the library you use thread that for you for when the back off would be called? I can try and explain in more detail what I mean if I'm still being to unclear (Threading can get confusing xD). My email is Turkey2349@gmail.com, feel free to wait till you get the formatter all sorted before you send it, I'm not in a dire need for it currently.

carlosCharz commented 6 years ago

@Turkey2349 I understand. I got your point about that nothing is preventing the Thread.wait if the send packet method fails. Let's move forward. Now, the question would be: Is the current code with backoff stop other packets to be sent? I guess no for other packets but yes for the current one that is waiting. I don't know if this is wrong. Did you get my point now?

TheTurkeyDev commented 6 years ago

If I were to send two packets one after the other and the first fails to send, is it not preventing/delaying the second packet from sending as well? In my case, I don't want my program hanging because a packet isn't sending. My overall question I guess is is that Thread.Sleep() call being made on the same thread that originally called sendPacket()

carlosCharz commented 6 years ago

Yeah I understand. You are totally right. But my understanding is that when you send the second packet it was handled in other thread by the application (done internally) since it is a smack listener.

So let me create a unit test to confirm this and come back with an answer.

Case: receive 3 packets where the first one fails. Configure a long delay so we can perceive if the second and third packet are sent. If so, I am correct and the code remains the same. If they wait until the first one finishes, you are right and we need to fix it. Is that ok?

TheTurkeyDev commented 6 years ago

Yes, exactly what I am getting at. The test sounds perfect. Let me know what you find!

carlosCharz commented 6 years ago

Look what I found:

https://github.com/igniterealtime/Smack/blob/master/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java

addAsyncStanzaListener method -> if you follow the code look for the threads Smack creates internally

TheTurkeyDev commented 6 years ago

Ok, so Smack handles the threading sweet. Yeah my code is redundant and unneeded then.

carlosCharz commented 6 years ago

But let me run my test. I will let you know the result anyway.

carlosCharz commented 6 years ago

Hi @Turkey2349 I tested and it works as expected. I already uploaded the backoff strategy to the main branch. Thanks for everything :)