Fix trasient typo; make general syntax and punctuation edits to readme #235

ColinDKelley · 2023-05-31T06:39:47Z

Fixed 'trasient` typo
Made a bunch of small syntax and punctuation edits

Types of Changes

Documentation

Contribution

I agree to the Developer's Certificate of Origin 1.1.

guides/asynchronous-tasks/readme.md

olleolleolle · 2023-05-31T06:48:51Z

@ColinDKelley Thanks for this big edit! Very nice formatting, and grammar improvements improve the clarity of the text.

guides/asynchronous-tasks/readme.md

ColinDKelley · 2023-05-31T06:46:56Z

guides/asynchronous-tasks/readme.md

@@ -124,7 +124,7 @@ end

 ## Waiting for Tasks

-Waiting for a single task is trivial, simply invoke {ruby Async::Task#wait}. To wait for multiple tasks, you may want to use a {ruby Async::Barrier}. You can use {ruby Async::Barrier#async} to create multiple child tasks, and wait for them all to complete using {ruby Async::Barrier#wait}.
+Waiting for a single task is trivial: simply invoke {ruby Async::Task#wait}. To wait for multiple tasks, you may either {ruby Async::Task#wait} on each in turn, or you may want to use a {ruby Async::Barrier}. You can use {ruby Async::Barrier#async} to create multiple child tasks, and wait for them all to complete using {ruby Async::Barrier#wait}.


We could still use some more guidance here about which is preferred and why.

We noticed that Async::Barrier#wait was less convenient because it didn't allow us access to the final values the way Async::Task#wait does. But we did find that Async::Barrier#wait performed a bit better in our use case. (We still need to boil this down to a stand-alone example.)

You can still call wait on individual tasks if required after calling Barrier#wait.

Depending on your use case, you should only follow that model if you are doing map-reduce. Explicit fan-out only to do a linear map of the results is a bit pointless - better to move all the mapping logic to the tasks themselves for the best concurrency.

I think we can elaborate more in the barrier.md documentation.

Ok, no changes in the follow-up here.

guides/asynchronous-tasks/readme.md

ColinDKelley · 2023-05-31T06:52:06Z

guides/asynchronous-tasks/readme.md

@@ -222,7 +222,7 @@ Async do
 end
 ```

-If you're letting individual tasks held by a barrier throw unhandled exceptions, be sure to call ({ruby Async::Barrier#stop}):
+If you're letting individual tasks held by a barrier raise unhandled exceptions, be sure to call ({ruby Async::Barrier#stop}) to stop the remaining tasks:


I didn't follow the "letting" part at first. I think I do now, so I can take a shot at clarification.

I think we can assume it's normal to let unhandled exceptions out. Especially SystemErrors like SyntaxError that won't be rescued by a plain rescue => ex.

I added some clarification in the follow-up.

guides/asynchronous-tasks/readme.md

ColinDKelley · 2023-05-31T06:58:28Z

guides/asynchronous-tasks/readme.md

@@ -364,21 +366,23 @@ require 'async'
 require 'thread/local' # thread-local gem.

 class TimeCache
-	extend Thread::Local
+	extend Thread::Local # defines `instance` class method that lazy-creates a separate instance per thread


I found the use of this gem was distracting from the example. Maybe we can think up a different example that doesn't have the thread-local complexity?

Or, alternatively, if thread safety is critical topic to address, maybe a separate section just for that point?

It's not possible to share tasks globally, so we need a per-thread cache (which is actually the best from a contention point-of-view). Maybe there is a better way we can demonstrate it.

ColinDKelley · 2023-05-31T06:58:57Z

guides/asynchronous-tasks/readme.md


 	def initialize
 		@current_time = nil
 	end

-	def current_time
+	def current_time_string


I had to read this a couple times to get the point that that we were caching the string and hence only needed 1-second granularity.

Maybe we can add that as a comment.

Updated in the lead-in paragraph in the follow-up.

guides/asynchronous-tasks/readme.md

ColinDKelley · 2023-05-31T07:04:51Z

guides/asynchronous-tasks/readme.md

 		@refresh ||= Async(transient: true) do
-			while true
+			loop do
 				@current_time = Time.now.to_s
 				sleep(1)
 			end


Re: the comment just below, I don't follow how an exception will get raised. I thought that would require us to call .wait on the task. Given that we're not doing that, doesn't Async(transient: true) do ... end return immediately without any chance of raising an exception when the reactor terminates all tasks?

And BTW, do you mean "terminates all transient tasks" there?

But also, I'm missing the significance of storing nil into the @refresh variable. The rest of the class doesn't use that. So the only effect would be if refresh! were called again, right? Was that the point?

The refresh! task should exist while the reactor exists, if the user calls a top level Async{TimeCache.current_time_string} after exiting the top level block, the task created in refresh! will exit. We want to ensure that the next time refresh! is called, the task is recreated. Hence, on exiting the task, we ensure the instance variable is cleared.

Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>

ColinDKelley · 2023-05-31T07:23:23Z

Thanks for this big edit! Very nice formatting, and grammar improvements improve the clarity of the text.

@olleolleolle You're welcome! My team has just been converting a whole service over from Synchrony to Async so we've been studying these docs very closely in the last week. :)

ioquatix · 2023-06-01T04:30:10Z

guides/asynchronous-tasks/readme.md


-There are various situations where you may want to stop a task ({ruby Async::Task#stop}). The most common case is shutting down a server, but other important situations exist, e.g. you may fan out multiple (10s, 100s) of requests, wait for a subset to complete (e.g. the first 5 or all those that complete within a given deadline), and then stop (terminate/cancel) the remaining operations.
+There are various situations where you may want to stop a task ({ruby Async::Task#stop}) before it completes. The most common case is shutting down a server, but other important situations exist, e.g. you may fan out multiple (10s, 100s) of requests, wait for a subset to complete (e.g. the first 5 or all those that complete within a given deadline), and then stop (terminate/cancel) the remaining operations.

 Using the above program as an example, we can 


Also stopped half way through a sentence lol.

I took a shot at finishing the sentence in the follow-up. :)

ioquatix · 2023-06-01T04:37:36Z

guides/asynchronous-tasks/readme.md

 end
 ```

-Upon existing the top level async block, the {ruby @refresh} task will be set to `nil`. Bear in mind, you should not share these resources across threads, doing so would need further locking/coordination.
+Upon existing the top level async block, the {ruby @refresh} task will be set to `nil`. Bear in mind, you should not share these resources across threads; doing so would need some form of mutual exclusion.


Suggested change

Upon existing the top level async block, the {ruby @refresh} task will be set to `nil`. Bear in mind, you should not share these resources across threads; doing so would need some form of mutual exclusion.

Upon exiting the top level async block, the {ruby @refresh} task will be set to `nil`. Bear in mind, you should not share these resources across threads; doing so would need some form of mutual exclusion.

Fixed in follow-up.

ioquatix

This is such a great improvement, I'll merge, it, and if you feel inclined to do a 2nd round of improvements, you have feedback.

ColinDKelley · 2023-06-01T05:37:11Z

if you feel inclined to do a 2nd round of improvements, you have feedback

Absolutely! I'll work on another PR tomorrow.

ColinDKelley added 2 commits May 30, 2023 22:03

Fix trasient typo

b98dc74

General syntax and punctuation edits to readme.

3a7052e

olleolleolle reviewed May 31, 2023

View reviewed changes

guides/asynchronous-tasks/readme.md Outdated Show resolved Hide resolved

olleolleolle reviewed May 31, 2023

View reviewed changes

guides/asynchronous-tasks/readme.md Show resolved Hide resolved

olleolleolle reviewed May 31, 2023

View reviewed changes

guides/asynchronous-tasks/readme.md Outdated Show resolved Hide resolved

ColinDKelley commented May 31, 2023

View reviewed changes

ColinDKelley and others added 2 commits May 31, 2023 00:09

Update guides/asynchronous-tasks/readme.md

12ddf81

Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>

Update guides/asynchronous-tasks/readme.md

d0a161f

Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>

ioquatix reviewed Jun 1, 2023

View reviewed changes

ioquatix approved these changes Jun 1, 2023

View reviewed changes

ioquatix merged commit 5423657 into socketry:main Jun 1, 2023
12 of 15 checks passed

ColinDKelley deleted the fix-trasient-typo branch June 2, 2023 01:56

ColinDKelley mentioned this pull request Jun 2, 2023

Follow up from fix trasient typo #236

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix trasient typo; make general syntax and punctuation edits to readme #235

Fix trasient typo; make general syntax and punctuation edits to readme #235

ColinDKelley commented May 31, 2023 •

edited

olleolleolle commented May 31, 2023

ColinDKelley May 31, 2023

ioquatix Jun 1, 2023

ColinDKelley Jun 2, 2023

ColinDKelley May 31, 2023

ColinDKelley Jun 2, 2023

ColinDKelley May 31, 2023 •

edited

ioquatix Jun 2, 2023

ColinDKelley May 31, 2023

ioquatix Jun 1, 2023

ColinDKelley Jun 2, 2023

ColinDKelley May 31, 2023

ioquatix Jun 1, 2023

ColinDKelley commented May 31, 2023

ioquatix Jun 1, 2023

ColinDKelley Jun 2, 2023

ioquatix Jun 1, 2023 •

edited

ColinDKelley Jun 2, 2023

ioquatix left a comment

ColinDKelley commented Jun 1, 2023 •

edited by ioquatix

	Upon existing the top level async block, the {ruby @refresh} task will be set to `nil`. Bear in mind, you should not share these resources across threads; doing so would need some form of mutual exclusion.
	Upon exiting the top level async block, the {ruby @refresh} task will be set to `nil`. Bear in mind, you should not share these resources across threads; doing so would need some form of mutual exclusion.

Fix trasient typo; make general syntax and punctuation edits to readme #235

Fix trasient typo; make general syntax and punctuation edits to readme #235

Conversation

ColinDKelley commented May 31, 2023 • edited

Types of Changes

Contribution

olleolleolle commented May 31, 2023

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

ColinDKelley May 31, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinDKelley commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioquatix Jun 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioquatix left a comment

Choose a reason for hiding this comment

ColinDKelley commented Jun 1, 2023 • edited by ioquatix

ColinDKelley commented May 31, 2023 •

edited

ColinDKelley May 31, 2023 •

edited

ioquatix Jun 1, 2023 •

edited

ColinDKelley commented Jun 1, 2023 •

edited by ioquatix