Fix actor not passing additional callback arguments #3196
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3196 +/- ##
==========================================
+ Coverage 75.11% 75.22% +0.10%
==========================================
Files 240 241 +1
Lines 19170 19244 +74
Branches 4325 4338 +13
==========================================
+ Hits 14400 14476 +76
+ Misses 4770 4768 -2
☔ View full report in Codecov by Sentry. |
|
Can you please write a test that this issue solves? While the unit test is great, I would like to see the actual bug this PR solves... |
|
P.S. thanks for taking the time to open this PR! |
|
Since this requires the usage of actors and in between thread commination, the only thing I can think of is a e2e test. |
|
Ok. I will write some tests as soon as I get a change... |
|
I have added the e2e test. |
|
Thanks!! Can you please verify that the test fails without the fix? |
|
I verified it and it was failing. I guess though that trust is not ehough? |
|
Trust is enough 😀 you can create a PR with the test only to show that it is failing, but it's not necessary. Thanks for all the hard work! |
| }); | ||
|
|
||
| for (const [, count] of Object.entries(callCountByUrl)) { | ||
| expect(count).toBeGreaterThan(1); |
There was a problem hiding this comment.
Greater than 3-4? If the timeout is for 5 sec and the cache is for 1 sec?
There was a problem hiding this comment.
The test could become flaky, it's making real network requests. Maybe we can try with 3?
src/util/actor.ts
Outdated
| @@ -175,13 +175,13 @@ export class Actor { | |||
| if (task.error) { | |||
| callback(deserialize(task.error)); | |||
| } else { | |||
| callback(null, deserialize(task.data)); | |||
| callback(null, ...(deserialize(task.data) as any[])); | |||
There was a problem hiding this comment.
I still don't fully understand how this solves the problem... I mean task.data can be anything, so why cache control and expiry are not part of it?
Also if we used to send the data as a single object, now it is spread, isn't it?
There was a problem hiding this comment.
task.data is the 1st argument after the err argument of the callback.
As you can see from the piece of code in Actor.ts in the below screenshot, the done callback is what is passed to the real function, and probably in some other places that I ignore right now.
Actor in fact proxies to-from the web worker and keeps a cache of the real callbacks to call on the other side of the message (postMessage and message listener). It calls the function that has the logic with its new callback (here the done) instead of the real callback. Then it passes the result back to the real callback.
Prior to this change, ...data was just data. Any other argument after data was just dropped. So if in my function I called callback(null, data, cacheControl, expires), it was exactly like calling done(null, data) (here data is the array buffer of the vector tile in the example I gave with addProtocol). This because the callback is this done that you can see in Actor.
As you can see also the spread is both in the actor while it calls the real callback, but also in the done callback where it acquires the arguments, so it's in both places. Probably to make things more clear we could rename task.data to task.args.
There was a problem hiding this comment.
args makes more sense yes.
What do you mean by other places?
There was a problem hiding this comment.
I have seen some other mention of cacheControl and expiry in the arguments of the callback passed to the function loading the geojson if I recall correctly.
There was a problem hiding this comment.
For geojson sources I mean.
There was a problem hiding this comment.
I have tried to take a look at renaming data to args, but I wouldn't feel safe in doing so because data and task are typed as any in a lot of places, and a refactor is not that easy... Maybe this is a task for the future when typing is a bit more strict in this repo.
There was a problem hiding this comment.
After reading the code a bit more and this change it seems too risky for me, as I don't see the full picture yet, and I'm not certain that this change doesn't break anything...
I would recommend first creating a PR which only add types so that we know we don't break anything (adding types doesn't change the code), and after that we can introduce this fix.
See the following comment for more information, there's also some implementation with types as far as I've seen in the contour repo: #3185 (comment)
cc: @msbarry
Sorry for dragging this, but better safe than sorry...
|
No need to close this, it can wait for the other PR I believe... |
|
I have added a new commit improving the typings in Actor. |
|
Due to this issue and PR, I have started to replace all the actor communication layer with types and promises. |
emanueleperuffo
force-pushed
the
fix-additional-callback-arguments
branch
from
24fac88 to
6d7c9b6
Compare
|
Ok I understand. However, would you mind giving a look at the last commit in my PR? I have implemented some more strict typing for the messages: one for the request, one for the response and one for the cancel, instead of having a generic Message that doesn't distinguish and contains all the fields. |
|
I took it a step forward and added promises etc. |
Before this change,
Actorwas not forwarding additional callback arguments and assumed the callback is always in the form(err, data) => {}.There are examples where the callback has more than 2 arguments. For example, in the
loadVectorTilefunction implementation, the callback that is passed directly to the actor is defined as(err?: Error | null, data?: ArrayBuffer | null, cacheControl?: string | null, expires?: string | null) => {}. In this case,Actoronly passesdata, forgettingcacheControlandexpires.Linked issue: fixes #3180
Launch Checklist
CHANGELOG.mdunder the## mainsection.