Add Toit version of the external messaging #2291
Conversation
lib/system/external.toit
Outdated
| import io | ||
| import rpc | ||
|
|
||
| class ExternalMessageHandler_ implements SystemMessageHandler_: |
There was a problem hiding this comment.
I'd move this below the public bits.
lib/system/external.toit
Outdated
| set-system-message-handler_ TYPE this | ||
|
|
||
| close -> none: | ||
| clear-system-message-handler_ TYPE |
There was a problem hiding this comment.
Should we clear the handler here? It would have to be nullable, which may be annoying.
lib/system/external.toit
Outdated
|
|
||
| externals_ := {:} | ||
|
|
||
| class External: |
There was a problem hiding this comment.
So this will naturally be called external.External? It's a bit weird.
Client, Target, Reference ...
lib/system/external.toit
Outdated
| pid := pid-for-external-id_ id | ||
| if pid == -1: return null | ||
| result = External.private_ pid id | ||
| externals_[id] = result |
There was a problem hiding this comment.
Not sure I understand the design here. You're caching instances of External, but they come with a close. That feels shaky. Can you ever reopen them?
There was a problem hiding this comment.
As discussed in person. That was all wrong.
should be better now.
| if not external-message-handler_: return | ||
| external-message-handler_.close | ||
| external-message-handler_ = null | ||
|
|
lib/system/external.toit
Outdated
|
|
||
| close -> none: | ||
| if is-closed_: return | ||
| clear-notification-handler |
There was a problem hiding this comment.
So if you close any External, you drop the shared external message handler?
lib/system/external.toit
Outdated
|
|
||
| set-notification-handler handler/Lambda -> none: | ||
| if is-closed_: throw "CLOSED" | ||
| if not external-message-handler_: |
There was a problem hiding this comment.
You only want one of these open as long as you have any External instances, no?
There was a problem hiding this comment.
was all wrong.
should be better now.
lib/system/external.toit
Outdated
| is-closed -> bool: | ||
| return is-closed_ | ||
|
|
||
| byte-message_ message/io.Data --copy/bool -> ByteArray: |
There was a problem hiding this comment.
static? Or a 'this is a helper' comment.
Maybe call it encode-byte-message_?
There was a problem hiding this comment.
Went with encode-message_.
| test c-lib #[99, 99] | ||
|
|
||
| c-lib.close | ||
|
|
lib/system/external.toit
Outdated
|
|
||
| constructor.private_ .pid .id: | ||
|
|
||
| static open id/string -> Client?: |
There was a problem hiding this comment.
Should we make it possible to set the handler when you open so you don't drop notifications?
lib/system/external.toit
Outdated
|
|
||
| constructor.private_ .pid .id .notification-callback_: | ||
|
|
||
| static open id/string --notification-callback/Lambda?=null -> Client?: |
There was a problem hiding this comment.
Maybe we should use on-notify or on-message rather than notification-callback?
lib/system/external.toit
Outdated
| bytes := encode-message_ message --copy=copy | ||
| return rpc.invoke pid function bytes | ||
|
|
||
| set-notification-callback callback/Lambda? -> none: |
There was a problem hiding this comment.
| set-notification-callback callback/Lambda? -> none: | |
| set-on-notify callback/Lambda? -> none: |
or
| set-notification-callback callback/Lambda? -> none: | |
| set-on-message callback/Lambda? -> none: |
lib/system/external.toit
Outdated
| if client.notification-callback_: | ||
| client.notification-callback_.call argument | ||
|
|
||
| static start-if-necessary -> none: |
There was a problem hiding this comment.
Maybe just merge the start/stop functions?
| clients.do: | client/external.Client | | ||
| e := catch: | ||
| client.request 0 #[99, 99] | ||
| expect-equals "EXTERNAL-ERROR" e |
There was a problem hiding this comment.
Should this be "EXTERNAL_ERROR" for consistency with other errors?
|
|
||
| static void __attribute__((constructor)) init2() { | ||
| printf("registering external handler 1\n"); | ||
| test_service_t* test_service = (test_service_t*)malloc(sizeof(test_service_t)); |
There was a problem hiding this comment.
Having a toit_malloc that does GC if necessary would make this a bit more robust. It actually doesn't matter here, because this only runs on startup, but it is something to consider for the other places where we need to allocate.
No description provided.