Skip to content

Fix attach/detach race condition#887

Merged
ikbalkaya merged 10 commits intoimprove-cifrom
attach_detach_race_issue
Jan 11, 2023
Merged

Fix attach/detach race condition#887
ikbalkaya merged 10 commits intoimprove-cifrom
attach_detach_race_issue

Conversation

@ikbalkaya
Copy link
Copy Markdown
Contributor

@ikbalkaya ikbalkaya commented Jan 5, 2023

This PR improves spec conformance for RTL4h and RTL5i
However I think the spec might be a bit misleading (Please correct me if I'm wrong)
In RTL4h, it writes

If the channel is in a pending state DETACHING or ATTACHING, do the attach operation after the completion of the pending request

I'm not sure if the attach operation should be done after an attach already happened (no need for adding a pending attach). Currently it just register a completion listener if provided and then returns which I think is the correct behaviour. I changed the behaviour when the channel was in DETACHING state so it wouldn't send an attach right away and add a pending attach request to be sent after completion of detach.

The same applies for RTL5i, but in reverse order

Closes #885

@github-actions github-actions bot temporarily deployed to staging/pull/887/javadoc January 5, 2023 12:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/887/javadoc January 6, 2023 09:41 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/887/javadoc January 6, 2023 10:00 Inactive
@ikbalkaya ikbalkaya changed the title Attach detach race issue Fix attach/detach race issue Jan 6, 2023
@ikbalkaya ikbalkaya marked this pull request as ready for review January 6, 2023 10:01
@ikbalkaya ikbalkaya self-assigned this Jan 10, 2023
@ikbalkaya ikbalkaya changed the title Fix attach/detach race issue Fix attach/detach race condition Jan 10, 2023
@ikbalkaya ikbalkaya changed the base branch from main to ignore_renew_auth_callback_test January 10, 2023 15:14
Comment thread lib/src/main/java/io/ably/lib/realtime/ChannelBase.java Outdated
Co-authored-by: Wojciech Dawiskiba <w.dawiskiba@nomtek.com>
@github-actions github-actions bot temporarily deployed to staging/pull/887/javadoc January 11, 2023 10:07 Inactive
@ikbalkaya ikbalkaya requested a review from davyskiba January 11, 2023 10:07
Base automatically changed from ignore_renew_auth_callback_test to improve-ci January 11, 2023 11:02
Copy link
Copy Markdown
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Minor comments that don't change the execution, but might improve the maintainability.

Comment thread lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java Outdated
Comment thread lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java Outdated
Comment thread lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java Outdated
@github-actions github-actions bot temporarily deployed to staging/pull/887/javadoc January 11, 2023 11:44 Inactive
Co-authored-by: Quintin Willison <q@qwuk.net>
@github-actions github-actions bot temporarily deployed to staging/pull/887/javadoc January 11, 2023 11:49 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Re-attach fails due to previous detach request

3 participants