Queue new API + docs #30

nomisRev · 2020-02-20T17:19:54Z

This PR exposes the new APIs from Queue, peek and try that do not block.
It also includes additional docs and tests for the new APIs, and a small restructuring.

It seems I have incorrectly merged ConcurrentQueue with CancelableQueue tho.
Any1 any idea how I keep the git history correctly? I'd happily re-open a new PR.

aballano · 2020-03-06T17:08:00Z

Any1 any idea how I keep the git history correctly? I'd happily re-open a new PR.

If you mean within this PR it's already done when you squash, just edit the message from here. If you mean from the master history then I don't think so because we would need to rewrite history by force-pushing, which would mean anyone with the branch might have conflicts.

nomisRev · 2020-03-07T13:04:00Z

@aballano that's not what I meant.
It looks like I renamed ConcurrentQueue to CancellableQueue, but I removed CancellableQueue and moved/refactored ConcurrentQueue.

Not sure how I can reflect that change properly in git. I retried with intermediate commits but got the same history.

Not critical, so ready for review. :)

1Jajen1

Can't say all that much about the code. Looks good though ^^

There are however a few nits on the documentation, which other than those reads quite well 👍

arrow-fx/src/main/kotlin/arrow/fx/Queue.kt

1Jajen1 · 2020-03-07T13:52:51Z

Oh btw is it possible to support bulk operations like flush or offerAll? Those could be useful if you don't want to start fibers for each value to add/remove.

nomisRev · 2020-03-07T13:57:36Z

Yes, we definitely can but I haven't attempted to implement them yet.

nomisRev · 2020-03-07T15:41:34Z

@1Jajen1 what behavior would you expect for the different backpressure strategies? I think the one described below would be quite easy to add.

offerAll(la: Iterable): Kind<F, Unit>

Unbounded#offerAll: always adds all immediately
Bounded#offerAll: blocks until all are added, adds every element as soon as soon as capacity available. This way you can add more than the capacity size.
Dropping#offerAll: adds all in order, respecting capacity
Sliding#offerAll: adds all in order, respecting capacity

tryOfferAll(la: Iterable): Kind<F, Boolean>

Works same as other try operators.

flushAll(): Kind<F, List>

For all strategies returns what is currently in Queue. takeAll and peekAll might be better named for these operations.

nomisRev

Thanks for doc suggestions!!

1Jajen1 · 2020-03-07T15:49:41Z

I'd expect it to work as if we'd repeat offer so that: offerAll(xs) == xs.traverse_ { a -> offer(a) } or similar, which includes all the blocking/dropping etc.

Guessing from how the strategies currently work what you suggested should be the same right?

The same cannot really hold for flush it would be more like a recursive tryTake until it reaches None, so flush should never block.

nomisRev · 2020-03-07T17:01:45Z

Right, that's exactly what I was thinking.

So no need to implement anything low level, I'll add these as derived methods to Queue with tests to verify what I mentioned above.

1Jajen1 · 2020-03-07T17:22:41Z

Right, that's exactly what I was thinking.

So no need to implement anything low level, I'll add these as derived methods to Queue with tests to verify what I mentioned above.

That depends, are these methods supposed to be atomic? Or better put does this hold: offerAll(xs).fork().followedBy(offer(x).fork()) => Queue(x + xs) or Queue(xs + x)?

Your name suggestions (missed them the first time) also make sense: takeAll peekAll both sound good to me.

nomisRev · 2020-03-07T17:24:46Z

xs.traverse_ { a -> offer(a) } is not atomic so no.

Shouldn't be too hard to implement atomically.

nomisRev

Thanks for the extensive review @1Jajen1!!! 🙏 👏 I'll be addressing the PR comments today, and I'll improve the test suite based on your suggestions!

nomisRev · 2020-03-10T08:17:26Z

arrow-fx/src/test/kotlin/arrow/fx/QueueTest.kt

+ "$label - empty queue takeAll is empty" {
+ forAll(Gen.positiveIntegers()) { n ->
+ IO.fx {
+ val q = !queue(n)
+ !q.takeAll()
+ }.equalUnderTheLaw(IO.just(emptyList()), EQ())
+ }
+ }
+
+ "$label - empty queue peekAll is empty" {
+ forAll(Gen.positiveIntegers()) { n ->
+ IO.fx {
+ val q = !queue(n)
+ !q.peekAll()
+ }.equalUnderTheLaw(IO.just(emptyList()), EQ())


Oh cool Github marks the range of your comments "Comment on lines +93 to +107" 😍

But this only makes sense if you can verify that it generated at least one empty queue (you'd have to generate lists of ints in that case).

All these queues are empty, I'll update for more clarity but n here is the capacity of the Queue.

arrow-fx/src/test/kotlin/arrow/fx/QueueTest.kt

nomisRev · 2020-03-10T09:40:27Z

arrow-fx/src/test/kotlin/arrow/fx/QueueTest.kt

+ }
+ }
+
+ "$label - can take and offer at capacity" {


What we're testing here is that you can tryOfferAll 2 items for a dropping queue with capacity 1 and 1 taker.

Without the taker being registered, tryOfferAll would have to drop one of the items offered and thus tryOfferAll would fail and return false.

Co-Authored-By: Alberto Ballano <aballano@users.noreply.github.com> Co-Authored-By: Jannis <overesch.jannis@gmail.com>

1Jajen1 · 2020-03-10T13:27:19Z

arrow-fx/src/main/kotlin/arrow/fx/Queue.kt

@@ -67,13 +67,13 @@ interface Dequeue<F, A> {
 fun peekAll(): Kind<F, List<A>>
 }

-/** A polymoprhic effect typeclass that allows [Dequeue]'ing values from a [Queue]. */
+/** A polymorphic effect typeclass that allows [Enqueue]'ing values from a [Queue]. */


Suggested change

/** A polymorphic effect typeclass that allows [Enqueue]'ing values from a [Queue]. */

/** A polymorphic effect typeclass that allows [Enqueue]'ing values to a [Queue]. */

Missed that on the first read through

…v-queue-part-2

nomisRev · 2020-03-11T15:17:40Z

@1Jajen1 after some discussion with @raulraja I decided we should not expose shutdown at this point.

It's not a pattern we follow in the other concurrency primitives, nor is it a pattern that we actively support. Which is exceptions for control flow, in this case cancelation flow.

On top of that Arrow Fx offers automatic cancelation support, and powerful combinators to prevent cancelation breaking and leaking to occur. So we should stay close to that.

Additionally, we can always come back to this and add shutdown. The other way around is more difficult.

EDIT: not all tests descriptions you gave were clear to me, but this should now cover all behavior I believe. Please also review the docs again, I added some more documentation. cc\ @aballano.

aballano · 2020-03-12T10:06:40Z

Saw a test failing in between commits, might be flaky

 arrow.fx.QueueTest > BoundedQueue - takeAll returns emptyList with waiting suspended takers FAILED
     arrow.fx.internal.TimeoutException: Duration(amount=5, timeUnit=SECONDS)

danimontoya

Minor nits 🎉 😄

arrow-fx/src/main/kotlin/arrow/fx/Queue.kt

danimontoya · 2020-03-12T11:50:00Z

arrow-fx/src/main/kotlin/arrow/fx/internal/ConcurrentQueue.kt

+ if (take != null) later { take(value) }.map { true }
+ else just(true)


Suggested change

if (take != null) later { take(value) }.map { true }

else just(true)

take?.let { later { take(value) }.map { true } } ?: just(true)

I don't find ?: clearer than if-else expression here.

danimontoya · 2020-03-12T11:57:25Z

arrow-fx/src/main/kotlin/arrow/fx/internal/ConcurrentQueue.kt

+ * [Surplus]: Contains a queue of values and two maps of registered id & offer/shutdown callbacks waiting to
+ * offer once there is room (if the queue is bounded, dropping or sliding).
+ *
+ * [Shutdown]: Holds no values, an offer or take in Shutdown state creates a QueueShutdown error.


Is this case missing? and QueueShutdown ?

Removed shutdown support, thanks for noticing!

aballano

I gave a look at the tests and they look good IMO

aballano · 2020-03-12T10:54:56Z

arrow-fx-test/src/main/kotlin/arrow/fx/test/laws/Law.kt

@@ -14,6 +22,18 @@ import io.kotlintest.shouldNot
 fun <A> A.equalUnderTheLaw(b: A, eq: Eq<A>): Boolean =
 shouldBeEq(b, eq).let { true }

+fun <A> IOOf<A>.equalUnderTheLaw(b: IOOf<A>, EQA: Eq<A> = Eq.any(), timeout: Duration = 5.seconds): Boolean =


why do we need this explicit equalUnderTheLaw? Cannot we test like before using the one above?

Yes, it's for a proper assertion message. The one implement through IO.toString is not very meaningful.

#30 (comment)

Co-Authored-By: danieh <danimontoya_86@hotmail.com>

…v-queue-part-2

nomisRev and others added 10 commits February 19, 2020 19:18

Merge new Queue internal impl with Queue

dbcdf4a

Add some additional tests for new behavior

af71858

Add more tests

4f83932

Merge remote-tracking branch 'origin/master' into sv-queue-part-2

2d2f695

Add Concurrent syntax for Queue

d32ec2d

Fix compileJmhKotlin

0e489f8

Merge branch 'master' into sv-queue-part-2

e98dc30

Fix documentation

e34098b

Merge branch 'master' into sv-queue-part-2

ecd4eec

Merge branch 'master' into sv-queue-part-2

f3ea469

nomisRev requested a review from a team March 5, 2020 12:57

aballano added this to the 0.10.x milestone Mar 5, 2020

aballano requested review from aballano and danimontoya March 6, 2020 12:12

Merge remote-tracking branch 'origin/master' into sv-queue-part-2

73efd48

nomisRev and others added 2 commits March 7, 2020 14:00

Update Queue tests

fa0f32a

Merge branch 'master' into sv-queue-part-2

34ef6f3

1Jajen1 approved these changes Mar 7, 2020

View reviewed changes

nomisRev commented Mar 7, 2020

View reviewed changes

nomisRev added 2 commits March 8, 2020 10:03

Add takeAll & peekAll

701ea3f

Add tryOfferAll

b777802

nomisRev and others added 3 commits March 10, 2020 11:07

First round of PR review

a53876d

Merge remote-tracking branch 'origin/master' into sv-queue-part-2

76e6fc4

Merge branch 'master' into sv-queue-part-2

d9b29ba

nomisRev commented Mar 10, 2020

View reviewed changes

Apply suggestions from doc lang review

2b903c2

Co-Authored-By: Alberto Ballano <aballano@users.noreply.github.com> Co-Authored-By: Jannis <overesch.jannis@gmail.com>

1Jajen1 reviewed Mar 10, 2020

View reviewed changes

nomisRev and others added 8 commits March 10, 2020 15:22

Fix offerAll cancelation

77abcc1

Merge branch 'master' into sv-queue-part-2

75861ec

Add docs to Dequeue

98f339f

Merge branch 'sv-queue-part-2' of github.com:arrow-kt/arrow-fx into s…

ee3faf7

…v-queue-part-2

Remove shutdown from Queue API

7b2df23

Remove LinkedMap and use LinkedHashMap instead

58d9eef

Refactor API to be Iterable instead of Collection

a8ab3e2

Add tests to check all strategies

cdd4a7f

nomisRev added 4 commits March 11, 2020 16:25

Add examples queue strategies

40657f3

Fix unresolved A Dequeue docs

99217bc

Fix Dequeue docs some more

61e7155

Fix Sliding KDoc example

a200d41

danimontoya approved these changes Mar 12, 2020

View reviewed changes

aballano reviewed Mar 12, 2020

View reviewed changes

aballano and others added 6 commits March 13, 2020 11:39

Merge branch 'master' into sv-queue-part-2

aa69fbe

Merge branch 'master' into sv-queue-part-2

2aa7ddf

Merge branch 'master' into sv-queue-part-2

ae81131

Apply suggestions from doc review

9e6c6c7

Co-Authored-By: danieh <danimontoya_86@hotmail.com>

Code review, remove Shutdown references

ff076bd

Merge branch 'sv-queue-part-2' of github.com:arrow-kt/arrow-fx into s…

ee1e753

…v-queue-part-2

aballano merged commit 2c0cdbe into master Mar 17, 2020

aballano deleted the sv-queue-part-2 branch March 17, 2020 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queue new API + docs #30

Queue new API + docs #30

nomisRev commented Feb 20, 2020

aballano commented Mar 6, 2020

nomisRev commented Mar 7, 2020

1Jajen1 left a comment •

edited

Loading

1Jajen1 commented Mar 7, 2020

nomisRev commented Mar 7, 2020

nomisRev commented Mar 7, 2020 •

edited

Loading

nomisRev left a comment •

edited

Loading

1Jajen1 commented Mar 7, 2020 •

edited

Loading

nomisRev commented Mar 7, 2020

1Jajen1 commented Mar 7, 2020 •

edited

Loading

nomisRev commented Mar 7, 2020 •

edited

Loading

nomisRev left a comment

nomisRev Mar 10, 2020

nomisRev Mar 10, 2020

1Jajen1 Mar 10, 2020

nomisRev commented Mar 11, 2020 •

edited

Loading

aballano commented Mar 12, 2020 •

edited

Loading

danimontoya left a comment

danimontoya Mar 12, 2020

nomisRev Mar 17, 2020

danimontoya Mar 12, 2020

nomisRev Mar 17, 2020

aballano left a comment

aballano Mar 12, 2020

nomisRev Mar 17, 2020

	/** A polymorphic effect typeclass that allows [Enqueue]'ing values from a [Queue]. */
	/** A polymorphic effect typeclass that allows [Enqueue]'ing values to a [Queue]. */

		if (take != null) later { take(value) }.map { true }
		else just(true)

	if (take != null) later { take(value) }.map { true }
	else just(true)
	take?.let { later { take(value) }.map { true } } ?: just(true)

Queue new API + docs #30

Queue new API + docs #30

Conversation

nomisRev commented Feb 20, 2020

aballano commented Mar 6, 2020

nomisRev commented Mar 7, 2020

1Jajen1 left a comment • edited Loading

Choose a reason for hiding this comment

1Jajen1 commented Mar 7, 2020

nomisRev commented Mar 7, 2020

nomisRev commented Mar 7, 2020 • edited Loading

offerAll(la: Iterable): Kind<F, Unit>

tryOfferAll(la: Iterable): Kind<F, Boolean>

flushAll(): Kind<F, List>

nomisRev left a comment • edited Loading

Choose a reason for hiding this comment

1Jajen1 commented Mar 7, 2020 • edited Loading

nomisRev commented Mar 7, 2020

1Jajen1 commented Mar 7, 2020 • edited Loading

nomisRev commented Mar 7, 2020 • edited Loading

nomisRev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nomisRev commented Mar 11, 2020 • edited Loading

aballano commented Mar 12, 2020 • edited Loading

danimontoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1Jajen1 left a comment •

edited

Loading

nomisRev commented Mar 7, 2020 •

edited

Loading

nomisRev left a comment •

edited

Loading

1Jajen1 commented Mar 7, 2020 •

edited

Loading

1Jajen1 commented Mar 7, 2020 •

edited

Loading

nomisRev commented Mar 7, 2020 •

edited

Loading

nomisRev commented Mar 11, 2020 •

edited

Loading

aballano commented Mar 12, 2020 •

edited

Loading