[Discussion] NIOAPNS: NIO-based Apple Push Notification Service

Server Proposals
,
7

Thanks! That looks like a good start.

I don't have lots of time right now so please excuse my telegraph style comments below :slight_smile:

  • I think there are too many top-level types. I think the API could benefit from nesting related types
  • why is it swift-nio-http2-apns? I think you should drop the http2, I'm sure that if APNS switched to say HTTP/1.1 (unlikely :stuck_out_tongue:) you'd then implement that too in this package
  • APNSResponse's API looks more complicated than necessary to me: You set response.data and if that fails, response.error gets set behind the scenes. I think there should be an initialiser init<Bytes: Sequence>(bytes: Bytes) throws where Bytes.Element == UInt8 and if the initialisation fails, it would just throw
  • the APNS payload type is called APSPayload, is there an N missing?
  • API docs would be great. For example there's a threadID, I believe that's for conversation threads or something?
  • why do we need the APNSNotificationProtocol protocol? Why don't we just pass the contained aps: APSPayload?
  • APNSRequestEncoder closes the Channel if something goes wrong. Usually a reusable ChannelHandler that is a NIO-citizen does not close the Channel itself. It rather uses context.fireErrorCaught(error) through the ChannelPipeline so that the user can choose what to do. In most cases, there's no recovery and the 'application handler' (usually the last handler in a pipeline) will close the connection. But in certain cases the user might be able to handle the error and then we shouldn't just throw away the channel.
  • APNSStreamHandler shouldn't be necessary, you can just use RequestResponseHandler which should be way more efficient and correct.
  • please do not @_exported import (in Exported.swift), this will break SemVer majorly
  • do we need the APNSSigner protocol? There is only one implementation (DataSigner). I think we could rename DataSigner to APNSSigner. The parameters don't actually need to be Data, you literally only get their pointer. If you just make the init(data:) and sign(digest:) arguments UnsafeRawBufferPointer then you can add Data, ByteBuffer, and Array<UInt8> convenience functions
  • this looks very dangerous:
#if (OPENSSL_VERSION_NUMBER < 0x10100000L) || !defined(LIBRESSL_VERSION_NUMBER)
typedef struct ECDSA_SIG_st {
    BIGNUM *r;
    BIGNUM *s;
} ECDSA_SIG;
#endif

because if defines a type that must match your system's OpenSSL version's definition of that struct. If it's not a perfect match then really bad things will happen.