Transient publishing for #164 #166
Conversation
lib/ably/realtime/channel.rb
Outdated
| error = Ably::Exceptions::ChannelInactive.new("Cannot publish messages on a channel in state #{state}") | ||
| return Ably::Util::SafeDeferrable.new_and_fail_immediately(logger, error) | ||
| end | ||
|
|
||
| if !connection.can_publish_messages? | ||
| error = Ably::Exceptions::MessageQueueingDisabled.new("Message cannot be published. Client is configured to disallow queueing of messages and connection is currently #{connection.state}") | ||
| error = Ably::Exceptions::MessageQueueingDisabled.new("Message cannot be published. Client is not allowed to queue of messages when connection is in state #{connection.state}") |
There was a problem hiding this comment.
"to queue of messages" -> "to queue messages"
There was a problem hiding this comment.
Thanks, good spot.
| if messages.length > Realtime::Connection::MAX_PROTOCOL_MESSAGE_BATCH_SIZE | ||
| error = Ably::Exceptions::InvalidRequest.new("It is not possible to publish more than #{Realtime::Connection::MAX_PROTOCOL_MESSAGE_BATCH_SIZE} messages with a single publish request.") | ||
| return Ably::Util::SafeDeferrable.new_and_fail_immediately(logger, error) | ||
| end |
There was a problem hiding this comment.
Where'd this MAX_PROTOCOL_MESSAGE_BATCH_SIZE requirement come from? Don't remember any discussion on this, and it's not in the spec afaict
There was a problem hiding this comment.
See fd267e6. I mentioned in that commit that this limit existed before, so I have kept it as it was. @paddybyers I believe we have a max ProtocolMessage size, but do we have / want any limit on how many messages can exist within a single ProtocolMessage?
| @@ -111,16 +111,12 @@ def fail_messages_awaiting_ack(error, options = {}) | |||
| end | |||
| end | |||
|
|
|||
| # When a channel becomes detached, suspended or failed, | |||
| # When a channel becomes suspended or failed, | |||
| # all queued messages should be failed immediately as we don't queue in | |||
| # any of those states | |||
| def fail_queued_messages(error) | |||
| error = Ably::Exceptions::MessageDeliveryFailed.new("Queued messages on channel '#{channel.name}' in state '#{channel.state}' will never be delivered") unless error | |||
| fail_messages_in_queue connection.__outgoing_message_queue__, error | |||
There was a problem hiding this comment.
fail_messages_in_queue connection.__outgoing_message_queue__, error
Not something you added in this pr, but: am I misunderstanding something, or does this fail every message in the connection queue if any channel becomes suspended or failed? That's not right, surely
There was a problem hiding this comment.
No, that is not correct, see https://github.com/ably/ably-ruby/pull/166/files/f7a524e00e64a47cf5c9d974dbcf621f5b82307a#diff-b8c5e1edac0c00c8db3f3e6ebcc8e749R125, only the messages for that channel. However, based on what we agree on ably/docs#470, I'll most likely need to modify this...
| @@ -102,7 +102,11 @@ def dispatch_protocol_message(*args) | |||
| if channel.attached? | |||
| channel.manager.duplicate_attached_received protocol_message | |||
| else | |||
| channel.transition_state_machine :attached, reason: protocol_message.error, resumed: protocol_message.has_channel_resumed_flag?, protocol_message: protocol_message | |||
| if channel.failed? | |||
| logger.warn "Ably::Realtime::Client::IncomingMessageDispatcher - Received an ATTACHED protocol message for FAILED channel #{channel.name}. Ignoring ATTACHED message" | |||
There was a problem hiding this comment.
Is there a spec for this? Having the client deliberately ignore what the server is telling it the channel state is is a bit of a break from our usual model (of the server deciding and telling the client), no?
There was a problem hiding this comment.
This wasn't a change of behaviour. This was raising an exception before because a channel is not allowed to transition from failed directly to attached. See
|
ably/docs#470 is merged, so I need to implement this change before this is ready for review again. |
mattheworiordan
force-pushed
the
transient-publishing-issue-164
branch
from
fdf2065 to
841ac62
Compare
mattheworiordan
force-pushed
the
transient-publishing-issue-164
branch
from
841ac62 to
a139e37
Compare
… instancing a Channel object See https://github.com/ably/docs/issues/468
…_BATCH_SIZE messages This limit was built into the Ruby library before, yet there were no tests. We need to review if we want to keep this limit in place at all moving forwards.
mattheworiordan
force-pushed
the
transient-publishing-issue-164
branch
from
a139e37 to
1dd646f
Compare
This PR adds support for transient publishing as defined in the spec atL:
I thought it would be useful to additionally see what's involved in support transient publishing from the proposed
Client#publishmethod defined at ably/specification#64. Interested to hear your views on this.