Repository navigation
worker: implement SharedArrayBuffer/MessagePort transferring - #106
Conversation
| // Create a SharedArrayBuffer object for a specific Environment and Context. | ||
| // The created SharedArrayBuffer will be in externalized mode and has | ||
| // a hidden object attached to it, during whose lifetime the reference | ||
| // count is increased by 1. |
Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. PR-URL: #106 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. PR-URL: #106 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
df2416b to
b8ff855
Compare
| ->HasInstance(lifetime_partner)) { | ||
| if (!source->IsExternal()) { | ||
| env->ThrowError("Found internalized SharedArrayBuffer with " | ||
| "lifetime partner object"); |
There was a problem hiding this comment.
Assume this is an internal bug if this happens? If so we should make it clear that this error should be reported back to us.
There was a problem hiding this comment.
Hm, I guess so, yes … you could mess with this using process.binding() but I don’t think that really counts :)
| // If this is an external SharedArrayBuffer but we do not see a lifetime | ||
| // partner object, we did not externalize it. In that case, there is no | ||
| // way to serialize it. | ||
| env->ThrowError("Cannot serialize externalized SharedArrayBuffer"); |
There was a problem hiding this comment.
Tests would be nice although not necessary. TypeError.
There was a problem hiding this comment.
Yeah, right. I think this is going to require an addon test
| return WriteMessagePort(Unwrap<MessagePort>(object)); | ||
| } | ||
|
|
||
| env_->ThrowError("Cannot serialize unknown type of host object"); |
There was a problem hiding this comment.
Tests would be nice although not necessary.
There was a problem hiding this comment.
Yeah, I know it’s kind of unfortunate that they are in a separate commit – the thing is, it just makes a lot more sense to test these methods when actually transferring data between workers…
| } | ||
| } | ||
|
|
||
| env_->ThrowError("MessagePort was not listed in transferList"); |
There was a problem hiding this comment.
Hm, at least Firefox throws a special DataCloneError, which is a DOMException and therefore a plain Error…
There was a problem hiding this comment.
DOMException isn't really encouraged for new errors any more. TypeErrors and other error types that map to JS are the future for the web. Plus we don't really care about compatibility, do we?
There was a problem hiding this comment.
Yeah, not really the point :) Still, why TypeError? I don’t really think the semantics here are “this thing has the wrong type”, more like, “you missed an entry in a list, please add it”, which doesn’t seem like a type mismatch to me…
Plus we don't really care about compatibility, do we?
Well … for MessagePorts/MessageChannels we kind of do? 😄 Maybe not in this first iteration but it should be doable to get some degree of compatibility
There was a problem hiding this comment.
Still, why
TypeError?
TypeError is used to indicate an unsuccessful operation when none of the other NativeError objects are an appropriate indication of the failure cause.
https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard-typeerror
Either way I just realized require('v8').Serializer.prototype._getDataCloneError is also Error, so I guess consistency trumps all.
There was a problem hiding this comment.
https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard-typeerror
I think that’s because the language spec isn’t really concerned with these kinds of errors?
Either way I just realized
require('v8').Serializer.prototype._getDataCloneErroris alsoError, so I guess consistency trumps all.
I mean … that’s because I wrote it that way. 😄 We definitely can change that if you think it makes sense
| } | ||
| seen_shared_array_buffers_.push_back(shared_array_buffer); | ||
| msg_->AddSharedArrayBuffer(reference); | ||
| return Just(i); |
There was a problem hiding this comment.
Is the returned ID actually used anywhere in Ayo code or is it just for V8?
There was a problem hiding this comment.
Only for V8, to match the objects, really… the matching call on the deserializing side is deserializer.TransferSharedArrayBuffer(i, sab);
|
Sorry about my late review... I've been kinda busy the past couple of days. |
Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106
Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106
Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106
Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106
These have already been reviewed by @Qard but I would like to give them a bit of public review time on their own :) They can be merged pretty soon, though.