Skip to content

Fix doFinally rx extension#29

Open
solomidSF wants to merge 1 commit intouber:masterfrom
solomidSF:feature/fix_for_do_finally
Open

Fix doFinally rx extension#29
solomidSF wants to merge 1 commit intouber:masterfrom
solomidSF:feature/fix_for_do_finally

Conversation

@solomidSF
Copy link
Member

@solomidSF solomidSF commented Sep 23, 2021

Expected behavior

Underlying peripheral queue should execute all operations in sequential manner.

The issue

Peripheral queue is not executing operations sequentially when there are more than 2 operations enqueued.
Receiving side would get interleaving chunks for different incoming packets.

Reason

public func queue<O: GattOperation>(operation: O) -> Single<O.Element> returns sequence with injected side-effects utilizing internal state to manage/run operation queue in case there are more operations to be executed. doFinally rx extension was added to enqueue next operation when the current one finishes, however it's not implemented correctly.
Rx lifecycle usually looking like this:

==...==completed==disposed=|
==...==error==disposed=|
Please note: subscription is disposed immediately after completed/error event occurs.

doFinally is called twice per single operation (e.g. completed, disposed) and due to side-effects utilizing internal state it skips freshly enqueued operation (which will still complete!) and moves to the next one.

Example:

  • Schedule 3 write operations
  • Write1 completes
  • doFinally is executed for Write1 because of the completed event so Write #2 is enqueued
  • doFinally is executed again for Write1 because subscription will be disposed so Write #3 will be enqueued
  • We are now sending packets both for Write2 and Write3 simultaneously

Added test to cover this behavior. Revert my change to doFinally to see test failing with previous logic: receiving side would get "Packet 1 containing dummy dataPackeP3t 2 containing slightly more dummy data than first packet" instead of "Packet 1 containing dummy dataPacket 2 containing slightly more dummy data than first packetP3"

Fix

Added variable to guard against multiple invocations.

@CLAassistant
Copy link

CLAassistant commented Sep 23, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@jpsoultanis jpsoultanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting a fix for this issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants