monitorSocket in ApnsConnectionImpl

[amitfrid]

Hi,
monitorSocket method is implements to run a new Thread in a
Asynchronous way. According to the code it should run only after a
Socket object is created.
The method itself checks for error codes on sending messages - so it
seems that it doesn't do what it was intended for (or do you get from
Apple an error code on opening a socket?)
Also, if error occurs, there isn't any code to close the socket.
Moreover, you might get a race condition if you send a message and the
thread already ran...
If I am right, is it possible to solve this issue, so the monitoring
will check for error code for every "error-detection-enabled" message
sent.

Thanks in advance,
Amit

[Mahmood Ali]

Greetings,

Thank you very much for raising all the concerns and reviewing the code. It helps us revise the library and hopefully improve it.

I don't quite understand the concern here. Can you elaborate? How is the method not doing what is intended? How can we have a race condition?

The Apple APNS protocol is designer for performance and not reliability or "debuggability." Please read the specification of the protocol to understand the code further.

Essentially, Apple doesn't return an acknowledgement for each message. Rather, at the first instance with an invalid message (or any error case), Apple servers drop the connection and return a 6 byte error code. In the life time of the entire connection, APNS servers never write to the connection except for 6-bytes indicating the error and the identifier of the erroneous message and then closes the connection immediately.

Because of such design, checking for the error code at every message sacrifices performance significantly. The library will need to wait to check for an error code for a time-out period before it can assume that the message was sent successfully. Because of the TCP protocol and some OS buffer issues, the time out period will be in the order of tens of microseconds or so. That will take the library throughput from >600 messages/second to <50 messages/second. I opted for an optimistic error checking (just send messages till we get an error and deal with it), rather than a pessimistic design (check for every message for an error).

Regards,- Mahmood

[amitfrid]

This is because the read() method will block for the lifetime of the
entire connection until first 6 bytes are returned?
Doesn't it have some kind of timeout for waiting for input?
I think that some kind of improvement is to actually check if the read
returns an error code or something else so the connection can continue
(and the thread returns to wait on read()) and in the other hand when
there is an error code returned to check that the socket actually
closed and did not stay in some other non-stable state (or simply kept
open...).

Thanks,
Amit

[Mahmood Ali]

Greetings,

> This is because the read() method will block for the lifetime of the
> entire connection until first 6 bytes are returned?

Correct!

> Doesn't it have some kind of timeout for waiting for input?

Not as far as I know. There are limitations related to being behind
NATs and such, where a connection dies silently, but that's a separate
problem and cannot be solved well unfortunately. The library wil just
detect it and restarts the connection.

> I think that some kind of improvement is to actually check if the read
> returns an error code or something else so the connection can continue
> (and the thread returns to wait on read()) and in the other hand when
> there is an error code returned to check that the socket actually
> closed and did not stay in some other non-stable state (or simply kept
> open...).

Can you elaborate on the proposal and be specific. What do you mean
by "if the read returns [..] or something else" or "connection can
continue". Please note that `sendMessage` is place where I check
whether a connection is closed or not and properly closes it.

Also, with Java API, there is no go mechanism to actually "check that
the socket actually closed"; the only reliable mechanism is to write
to a channel and see whether the write succeeded or not (and that's
not perfect either). So basically, you'll have just to close it,
which is what is done in sendMessage() in the main thread. You may
want to paraphrase the sentence as "force close the socket", rather
than "check".

Feel free to submit a patch to improve it.

- Mahmood

[amitfrid]

Hi,
To be more specific:
1. You are assuming that in Apple protocol the returned 6 bytes are
parsed into one of the Enum values of DeliveryError. Otherwise an
exceptoin will be thrown (won't call delegate.connectionClosed()) -
this can be improved by a try-finally block
2. You are assuming that Apple side closed the connection after
sending the 6 bytes. If it will be changed one day, you are calling
delegate.conenctionClosed() without actually check if the socket is
closed. Can be improved by calling socket.isClosed(). Maybe if it
wasn't closed you want to force close it to create a new socket?

Thanks,
Amit

[amitfrid]

Hi,
I am running an app that runs 5 ApnsService (version 0.1.5) service
instances, which I am using with ArrayBlockingQueue in a cyclic way.
After sending 5 push messages I assumes (and verified it in the log)
that all instances were in use once.
I am sending well-formed pusher messages that I can see in that are
received in my iPhone. I am keeping several seconds delay between each
send.
Later, in purpose, I've sent 5 bad-formed messages (over 256 bytes) -
in order to monitor the connection was closed.
and then again - 5 well-formed.

After the send, I am running a thread dump. The expectation was to
have 5 threads of MonitorThread kind that are waiting for and error to
be received on the socket.
I didn't see any of the threads. moreover, it seems that the
connections weren't closed, but the bad-forms messages weren't arrived
on my iPhone.
I wrote a Delegate object in order to listen to messageSent,
messageFailed and connectionClosed event.
I actually got 15 messageSent events only.

Below is my code to create an ApnsService. I can send you the thread
dump in private (didn't find a way to do it here). If you need more
information let me know.
p.s might be related to working with Sandbox?

String location = <location to my app certificate>
InputStream certStream =
this.getClass().getClassLoader().getResourceAsStream(location);
MyNotnoopDelegate apnsDelegate = new MyNotnoopDelegate(this); //my
private Delegate object
ApnsService apnsService =
APNS.newService().
withCert(certStream, password).
withSandboxDestination().
asPool(2). //thread pool in size 2 for the scenario where message
failed and other thread can continue until origial thread wakes (1
sec)
withDelegate(apnsDelegate).
withReconnectPolicy(ReconnectPolicy.Provided.EVERY_HALF_HOUR).
build();
apnsService.start();


.Thanks,
Amit

[amitfrid]

Hi,
I found the cause for the problem and it is a follow:
I am creating a APNSService with pooled connection. The pooled
connection wraps the apnsConnection object.
The wrapping creates a copy of ApnsConnection (calling the copy()
method) saved in the object 'uniquePrototype' in ApnsPooledConnection.
The copy method does not copy the value of errorDetection field, and
is calling a c'tor which set it to always false. So from default true
which is set with the APNSServiceBuilder.build(), it became false and
the MonitorThread will never be created.
That means that ApnsPooledConenctoin doesn't support the error
detection mechanism.
I can see it already fixed in version 0.2.0

Thanks,
Amit