Use task source mechanism to dispatch events instead of firing them synchronously

[alexshalamov]

Short summary:

Sensors are operating in simple Publish & Subscribe model, therefore, queued event processing is not required and over-complicates API and it's implementation.

Detailed explanation:

Event queue is suitable for cases where ordered task processing is needed. For example, networking, databases or APIs that provide sync API and manage async task queue. You can call write(data); multiple times and each write is ordered task, if error happens, all pending tasks are removed from queue and 'onerror' event task is put to the queue.

Problems with queued task processing:

• The 'onchange' event would need to carry reading data, otherwise when event queue is processed, event would not be in sync with Sensor.|reading|
• When we attach |reading| to an event, we will have another problem, event.reading != Sensor.reading, might be confusing for developers.

Pros for simple event:

• onchange is synchronized with Sensor.reading
• No need to carry extra data in event.
• No need to manage queue, we don't even need a queue, since we always have 1 update per event loop cycle per Sensor instance.
• Simpler for browser vendors to implement.

Proposed resolution:

• Use simple event dispatch mechanism
• Remove task source related information from the spec

[alexshalamov]

@tobie @pozdnyakov @anssiko ptal

[tobie]

That's a great insight. I need to read-up more on ho task sources actually function to be able to make an educated comment here.

You might have noted that I have an inline issue on that topic in the spec. I'll link to this instead.

[pozdnyakov]

I think that reflects also the actual implementation in Chromium..

[tobie]

That said:

Use simple event dispatch mechanism

I think that is what the spec is currently doing, no?

Remove task source related information from the spec

Afaik, the task source was brought in (by @anssi) only as a mechanism to deal with mitigation strategies, it can be added differently.

As mentioned above, I know little about how it works or the difference between the two.

[anssiko]

With the new information in #215 (comment), the suggestion I made in #81 (comment) is no longer valid. @tobie, I'm fine removing the task source concept from the spec.

[alexshalamov]

@tobie @anssiko Do you want me to create PR that will fix this issue?

[tobie]

No thanks. I'll fix it as part of #213.

[alexshalamov]

No thanks. I'll fix it as part of this PR.

@tobie Why? This issue has no direct relation to #213 (it is for mitigation strategy). Would be nice to have 1 PR for 1 issue whenever possible.

[tobie]

Would be nice to have 1 PR for 1 issue whenever possible.

That's at the discretion of the editor. ;)

[tobie]

OK, so after discussing this in #whatwg, it seems that whole Update Reading algorithm should happen in a new task, so we avoid updating the shared buffer in the middle of a script being processed, in which case, gyro.x === gyro.x would not be guaranteed to always return true.

Once in this task, however, we could continue firing the events directly (as we now do).

[alexshalamov]

Shared buffer is not exposed to JS side, it is implementation detail and internal slot. In chromium implementation there are no side-effects related to updating it during script execution, thus gyro.x === gyro.x should be true during event-handler execution. This is fixed by #197 or by #210

[tobie]

Shared buffer is not exposed to JS side, it is implementation detail and internal slot.

As an internal slot, it can still have side effects on the JS side.

In chromium implementation there are no side-effects related to updating it during script execution, thus gyro.x === gyro.x should be true during event-handler execution.

But this is implementation specific, it is not specified by the spec, is it?

This is fixed by #197 or by #210

I don't think it is, tbh. Maybe switching steps 6 and 7 of the "Update latest reading" algorithms would do that. But overall, I think we could replace this whole "reporting flag thing" by simply adding the whole "Update latest reading" in a new task.

[alexshalamov]

As an internal slot, it can still have side effects on the JS side.

Then it would be bug for the browser that introduces such side effects, internal implementation should not affect specified behavior.

But this is implementation specific, it is not specified by the spec, is it?

Well, we had consensus that it should be true for event loop turn. If that is not in the spec, maybe we should reflect that agreement, do you want me to make PR for that? 😄

I don't think it is, tbh.

I think tbh :) and we have tests for that that actually work.

I think we could replace this whole "reporting flag thing" by simply adding the whole "Update latest reading" in a new task.

• Do you want to keep task source event processing model?
• New task for what task queue, from what task source?
• Do you know how this is implemented in browsers?
• Do you know why IDB or WebSockets use task queues?
• Do you know how to fix sync side-effects when we have single sensor.reading and multiple tasks in queue?

As I mentioned in this issue, event queues are overkill for such simple event processing model.

[alexshalamov]

Unfortunately, I don't remember whether this commit was reviewed by anyone.

[tobie]

Unfortunately, I don't remember whether this commit was reviewed by anyone.

Fwiw, that's a word for word copy of text submitted by @anssiko here: #81 (comment).

That said, depending on the context, editing process of specs can be a lot looser than that of code. What's important is for the editor to garner group consensus. It's pretty obvious from the above link that it was the case. I'm not quite sure what point you're trying to make, here nor why you're responding so defensively to my comments. My goal here is to make sure that we have a solid spec. Nothing else.

[tobie]

As an internal slot, it can still have side effects on the JS side.

Then it would be bug for the browser that introduces such side effects, internal implementation should not affect specified behavior.

I meant that changing it in the middle of an event turn could be observable in JS. Sorry for the confusion.

But this is implementation specific, it is not specified by the spec, is it?

Well, we had consensus that it should be true for event loop turn. If that is not in the spec, maybe we should reflect that agreement, do you want me to make PR for that? 😄

I'm not sure what you mean. I don't recall discussing this before. Nor should it be a topic of consensus. Having an attribute change value in the middle of an event turn isn't something you want to see on the platform at all. This is not subject to discussion.

Would you be confusing this issue with slowSensor.x === fastSensor.x which has been the subject of a number of discussions?

If not, before submitting a PR, could you please enlighten me on how you would solve this besides the step-switching I suggested above?

I don't think it is, tbh.

I think tbh :) and we have tests for that that actually work.

Well again, I'm not arguing your implementation doesn't do the right thing. I'm saying I don't think the spec guards against this issue, but as I said, this is an area where I don't have a great understanding of how the spec works. The people I talked about this on the #whatwg seemed to imply that we should be calling the whole thing from a task to avoid the latest reading changing mid-script.

I think we could replace this whole "reporting flag thing" by simply adding the whole "Update latest reading" in a new task.

Do you want to keep task source event processing model?

We don't have that now. So I'm not sure what you mean.

New task for what task queue, from what task source?
Do you know how this is implemented in browsers?
Do you know why IDB or WebSockets use task queues?
Do you know how to fix sync side-effects when we have single sensor.reading and multiple tasks in queue?

Nope. But you seem to know a lot about this stuff, so maybe you can help address my concerns, no?

As I mentioned in this issue, event queues are overkill for such simple event processing model.

Yeah, the people I talked to didn't seem to think event queues would be useful here either. They just suggested having a new task for the Update latest reading algorithm. At least that what I understood.

[alexshalamov]

I'm not quite sure what point you're trying to make, here nor why you're responding so defensively to my comments.

Sorry if I sounded defensively, I had no such intention. I like facts based, technical approach when decisions are made. So, I'm asking lots of 'why' to understand what were the technical aspects for some decisions that we have in the spec at the moment.

My goal here is to make sure that we have a solid spec.

We have same goal, that's why we are trying to help by pushing PR's, reviewing PRs, actively participating in all discussions, implementing spec in browser, etc.

[alexshalamov]

I'm not sure what you mean. I don't recall discussing this before.
w3c/ambient-light#15
#147

[tobie]

I'm not sure what you mean. I don't recall discussing this before.
w3c/ambient-light#15
#147

Yeah, that's not the same issue at all. This was about the reading object being immutable. A completely different story. Here we're talking about the shared buffer changing value in the middle of an executing script.

[tobie]

For reference, here's the log of our conversation on #whatwg: http://logs.glob.uno/?c=freenode%23whatwg#c1029330

[alexshalamov]

I meant that changing it in the middle of an event turn could be observable in JS. Sorry for the confusion.

No problems, sorry, maybe I was also unclear. Shared buffer is internal implementation detail and internal slot, good implementation would treat it this way. External behavior must be same regardless of implementation details. Thus, in chromium, if shared buffer is updated in the middle of the script, it will not be observable on JS side, e.g., sensor.reading === sensor.reading should be respected. That's why @pozdnyakov is fixing it in #210, to sync spec with agreement we made.

Yeah, that's not the same issue at all. This was about the reading object being immutable.

Was there a time when we had issue just about one thing 😃 ?

Rick proposed quite nice set of tests and everyone agreed, issue closed and implemented in chrome according to agreement.

// In this turn, sensor.reading attribute's value will always be the same SensorReading object
console.log(sensor.reading === sensor.reading);

For reference, here's the log of our conversation on #whatwg: http://logs.glob.uno/?c=freenode%23whatwg#c1029330

Thanks, you asked on irc about other specs, you might want to check IDB, WebSockets and if I remember correctly MediaStream interface.

Do you want to keep task source event processing model?

We don't have that now. So I'm not sure what you mean.

We have mixed model :D Sensor is a task source that should add events to the queue, and it should stop (start) queue processing when visibility changes.

[tobie]

Shared buffer is internal implementation detail and internal slot, good implementation would treat it this way. External behavior must be same regardless of implementation details.

Well, I agree. But good spec works makes sure that it is spec'ed properly.

Thus, in chromium, if shared buffer is updated in the middle of the script, it will not be observable on JS side,

Yeah, I'm not suggesting your implementation has issues. Just that the spec isn't clear about that and that, according to the thread mentioned above, using a task (on the spec side) is the way to handle this.

e.g., sensor.reading === sensor.reading should be respected.

Well, it's a concurrency issue, so sensor.reading === sensor.reading could very well return true most of the time.

That's why @pozdnyakov is fixing it in #210, to sync spec with agreement we made.

Could you point me to where in the spec this is fixed. I fail to see how anything in that PR fixes it (which doesn't mean it's not fixed, just I fail to see it).

Was there a time when we had issue just about one thing 😃 ?

😂 😂 😂

you asked on irc about other specs, you might want to check IDB, WebSockets and if I remember correctly MediaStream interface.

Not sure what the consensus about WebSockets is nowadays (but it's a pretty old spec), but I don't think either IDB or MediaStream has a good reputation in terms of being properly spec'ed.

We have mixed model :D

Yeah—I know. The backstory is that I added @anssiko's work in there to meet a deadline, knowing very well we'd have to tackle the task issue heads on at some point. Hence the inline issue on the topic I added later. Technical debt.

[anssiko]

Re technical debt, now that we have a tighter feedback loop with the implementation similar accidents are less likely to happen again.

[alexshalamov]

Could you point me to where in the spec this is fixed. I fail to see how anything in that PR fixes it (which doesn't mean it's not fixed, just I fail to see it).

Sure, report latest reading updated algorithm and update sensor reading that makes deep copy of latest reading to Sensor.reading, therefore, Sensor.reading is not pointing directly to internal shared buffer (latest reading), thus, modification of latest reading in the middle of script execution would not modify Sensor.reading.

• Sensor.reading === Sensor.reading during event handler execution
• SensorA with different frequency would not modify SensorB behavior
• Reporting frequency is respected, e.g., Sensor.reading is not updated faster than requested.

Yeah—I know. The backstory is that I added @anssiko's work in there to meet a deadline, knowing very well we'd have to tackle the task issue heads on at some point. Hence the inline issue on the topic I added later. Technical debt.

Technical debt is fine, as long as we are taking care of it. Thanks for involving #whatwg and asking for hints!

[tobie]

@anssiko:

Re technical debt, now that we have a tighter feedback loop with the implementation similar accidents are less likely to happen again.

Feedback from implementors is great, but that particular issue was completely orthogonal, tbh.

[tobie]

So definitive feedback from WHATWG folks is roughly to write:

1.  [=In parallel=], wait for a a new reading to be available.
    When it is, [=queue a task=] to run the following sub-steps:
    1.  If [[unsecure_stuff_happening]] flag is set, then return.
    1.  Update the [=latest reading=] map.
    1.  Copy to instance cache if that's what we end-up choosing to do.
    1.  Fire a "change" event.

So we can get rid of the custom task queue, but not of task queues altogether.

[tobie]

For further reference: http://logs.glob.uno/?c=freenode%23whatwg#c1029373, including Anne who says:

Do not update state without a task

[alexshalamov]

Anne, smaug and jgraham had good valid points. Let’s keep task source and align spec with it to avoid ‘mixed mode’. I will create several smaller issues that I identified and update this comment.

#216
#217
#218
#220