Update parallelism.d

[Imperatorn]

Make TaskStatus public

[dlang-bot]

Thanks for your pull request and interest in making D better, @Imperatorn! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8832"

[jmdavis]

What is the goal here? From what I can see, TaskStatus is only used internally and isn't exposed at all in std.parallelsm's API. As such, I don't see any reason why it would make sense for it to be public. It's just an implementation detail.

[Imperatorn]

The failing tests are because of "Monthly free compute limit exceeded!"

[Imperatorn]

What is the goal here? From what I can see, TaskStatus is only used internally and isn't exposed at all in std.parallelsm's API. As such, I don't see any reason why it would make sense for it to be public. It's just an implementation detail.

The goal is to be able to use the status and present it.

For example just knowing that the status is 1 or 2 doesn't say so much, but saying that the status is inProgress, done etc means something.

[jmdavis]

As far as I can see, the status is not exposed anywhere in the API. So, unless I'm missing something, as things stand, there's no point in making TaskStatus public.

As such, if you want to make the status part of the public API, then it would make more sense to create a PR which exposes the status as part of the API so that those changes can be reviewed as a whole rather than simply making TaskStatus public.

[Imperatorn]

As far as I can see, the status is not exposed anywhere in the API. So, unless I'm missing something, as things stand, there's no point in making TaskStatus public.

As such, if you want to make the status part of the public API, then it would make more sense to create a PR which exposes the status as part of the API so that those changes can be reviewed as a whole rather than simply making TaskStatus public.

But if it was public and I were to make it private, you would object to that instead.

[jmdavis]

In general, we can't make public symbols private, because it will break existing code, and in that same vein, making things public when they don't need to be just adds symbols to the public API that we then need to worry about breaking when making changes to the implementation. So, in general, we need to be very careful about what in Phobos' API is public and what isn't.

It can make perfect sense to make TaskStatus public if the API exposes the status in a sensible way, but unless I'm missing something, right now, nothing in the public API uses TaskStatus at all. As such, making it public adds no value. It just pointlessly exposes a symbol that is currently only an implementation detail of the module.

If you create a PR that adds the status to the API, then it would make sense to make TaskStatus public as part of that, and the changes can be reviewed as a whole for whether they make sense. But simply making TaskStatus public without any of the other changes is pointless.

[Imperatorn]

There's nothing wrong with the API, so why would I change it? It wouldn't really make any sense.

The type is not exposed, it can't be accessed.

Even if I were to add a getStatus which could return a TaskStatus doesn't change the problem. You would still not have access to the TaskStatus so it wouldn't add any value.

How would getting the status be possible if exposing what it means is forbidden?

[jmdavis]

If you create a PR that creates a getStatus function, then you can make TaskStatus public as part of that PR, and then the API changes can be reviewed as a whole. There is no need to create separate PRs for making TaskStatus public and for code that then actually uses it in the public API.

[Imperatorn]

I can change it so that the AbstractTask exposes inProgress and notStarted so that Task can provide them as part of the new API. It will need a little more work but ok.

[Imperatorn]

I have updated the PR and made the changes you requested

[Imperatorn]

With this change, TaskStatus can stay private. I can change it tomorrow if needed. I also want to add some more documentation about exception in task.

[braddr]

The problem with this change isn't the public vs private-ness of it. The problem is that there's more or less no way to use the added API's safely/correctly. They introduce classic race conditions and end up meaning something other than what the API names suggest. inProgress ends up meaning is either running or done and notStarted ends up meaning any state. There's no way to atomically ask the question and act on the answer with these API's.

[jmdavis]

The problem with this change isn't the public vs private-ness of it. The problem is that there's more or less no way to use the added API's safely/correctly. They introduce classic race conditions and end up meaning something other than what the API names suggest. inProgress ends up meaning is either running or done and notStarted ends up meaning any state. There's no way to atomically ask the question and act on the answer with these API's.

When he created the PR, all it did was make TaskStatus public, so there wasn't anything of substance to review, and the change was utterly pointless. At least now, the PR has changes that actually do something and can be reviewed based on how they affect the API and would potentially be used.

And yes, I would agree with your assessment. Asking whether the task has started or not isn't of much benefit, because even if the answer is no, by the time you actually look at the answer, the task could have started, meaning that you'd be acting on incorrect information. Similarly, asking whether it's in progress doesn't tell you much of anything, because by the time you look at the answer, it could be wrong. And unless I'm missing something, you can't actually do anything with the answer except log it, because the API is pretty much set up to just call the function on another thread and give you the result. You can't stop it, and without locking, any information you have on its state before it finishes isn't necessarily correct. And the API already has a function for telling you whether the task is done.

I guess that in principle, you could choose whether you want to call one of the *Force functions based on whether the task has started or not, but you'd potentially be acting on out-of-date information, and I don't know how whether a task has started or not would meaningfully inform a decision on whether to call one of the *Force functions. It's just the only thing that I can see where you even might do something based on that information. The API really isn't designed to let you do much of anything based on the information of whether the task has started or not. And given the locking situation, in order for it to give you the ability to do something based on that information, it looks like it would need a new function that would do something specific based on that information (where it could actually deal with whatever locking would be necessary to make it work) rather than returning the state information to you where you can't do any sort of locking to fix issues with race conditions.

So, there needs to be a clear use case as to why these functions would actually be useful, and I suspect that there would be a better way to handle that use case, but without knowing what's trying to be solved here, it's difficult to know what the proper way to solve the actual problem is. And these functions seem like they would just promote misunderstandings of what was going on, since it would be easy for a programmer to assume that the response from a function called notStarted would actually be telling them that it hadn't started if they didn't sit down and think through what was going on thoroughly enough to realize that it can't possibly be telling them for sure whether it's started or not.

So, Imperaton, what do you expect to actually be able to do if you have these functions that you can't do right now? If it's simply to present the status, why would that be useful? Particularly in light of the fact that a decent amount of the time, the answer is going to be outdated?

[Imperatorn]

The problem with this change isn't the public vs private-ness of it. The problem is that there's more or less no way to use the added API's safely/correctly. They introduce classic race conditions and end up meaning something other than what the API names suggest. inProgress ends up meaning is either running or done and notStarted ends up meaning any state. There's no way to atomically ask the question and act on the answer with these API's.

Which is why I just made the type public first

[tgehr]

As far as I can see, the status is not exposed anywhere in the API. So, unless I'm missing something, as things stand, there's no point in making TaskStatus public.
...

It's exposed via alias this.

import std.parallelism, std.stdio;

void main(){
	Task!(x=>x,int) task;
	writeln(task.taskStatus); // 0
}

This may have been by accident.

[Imperatorn]

As far as I can see, the status is not exposed anywhere in the API. So, unless I'm missing something, as things stand, there's no point in making TaskStatus public.
...

It's exposed via alias this.

import std.parallelism, std.stdio;

void main(){
	Task!(x=>x,int) task;
	writeln(task.taskStatus); // 0
}

This may have been by accident.

I am already using that.

What I have been trying to say not multiple times is, from taskStatus you just get a ubyte.

[Imperatorn]

Information refers to data that is processed or organized in such a way that it becomes meaningful and useful. A classical definition is something that reduces uncertainty. It provides context for data and enables decision-making. For instance, raw data can be simple numbers or facts, but when that data is interpreted and given a purpose, it becomes information.

Being given a value of 0, 1 or 2 does not provide any information by definition, and is therefore just data.

Hence, to be able to get actual information from the taskStatus, the enum must be available.

I already know I can get taskStatus, I thought this was obvious since why would I otherwise make the PR?

[Imperatorn]

Note, only the original commit should be merged

[cyrusmsk]

As @tgehr stated in Discord the whole access to the taskStatus outside of the internal implementation was made probably by accident.

[schveiguy]

Thanks for your bug report! Typically, we do bug reports through https://issues.dlang.org, for future reference, not via created PRs that hint at a bug in a roundabout way.

I've gone ahead and reported your bug for you: https://issues.dlang.org/show_bug.cgi?id=24207

[Imperatorn]

Thanks for filing an issue @schveiguy. For some reason I very often get DNS problems when trying to navigate to bugzilla

[Imperatorn]

[WalterBright]

issues.dlang.org is back up now.