Repository navigation
feat(durable-streams): Go, Python, Ruby SDKs - #5348
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mrkaye97
left a comment
There was a problem hiding this comment.
left a couple comments on the public API, I think if we make this async-only in Python it'll be dramatically simpler. I wish we'd made more stuff async-only early on...
| def _iterate_in_thread(agen: AsyncGenerator[T, None]) -> Generator[T, None, None]: | ||
| """Iterates an async generator from sync code on an event loop of its own.""" | ||
| loop = asyncio.new_event_loop() | ||
| thread = threading.Thread(target=loop.run_forever, daemon=True) | ||
| thread.start() | ||
|
|
||
| reading: asyncio.Task[T] | None = None | ||
|
|
||
| async def next_item() -> T: | ||
| nonlocal reading | ||
| reading = asyncio.current_task() | ||
| return await agen.__anext__() | ||
|
|
||
| async def close() -> None: | ||
| ## interrupted mid-read: a running generator can't be closed until the read unwinds | ||
| if reading is not None and not reading.done(): | ||
| reading.cancel() | ||
| await asyncio.wait([reading]) | ||
| await agen.aclose() | ||
|
|
||
| try: | ||
| while True: | ||
| try: | ||
| item = asyncio.run_coroutine_threadsafe(next_item(), loop).result() | ||
| except StopAsyncIteration: | ||
| return | ||
| yield item | ||
| finally: | ||
| asyncio.run_coroutine_threadsafe(close(), loop).result() | ||
| loop.call_soon_threadsafe(loop.stop) | ||
| thread.join() |
There was a problem hiding this comment.
I this is probably a bad idea - we've tried this before. IMO, we should probably make this async-only and not suffer from event loop blocked nightmares. WDYT?
| await asyncio.wrap_future( | ||
| self._enqueue_publish(topic, message, namespace, batch_timeout) | ||
| ) |
There was a problem hiding this comment.
it'd be ideal to have an async version of this because doing it this way will cause a ton of thread / event loop churn
| """ | ||
| return _iterate_in_thread(self.aio_events(topic, namespace, cursor)) | ||
|
|
||
| async def aio_events( |
There was a problem hiding this comment.
should we call this consume or subscribe? I tend to believe that function names should be verbs, and those both describe this better than events does IMO
[Medium impact] The PR adds durable-stream workflows across SDK clients and serverless task code. The PR is not ready to merge because the earlier Go ordering, Python cancellation, and Ruby 3.1 failures remain. SummaryAdds durable stream clients for Go, Python, and Ruby.
Reviews (4) · Last reviewed commit: "generate" · Reviewed by Greptile |
| if c.streams == nil { | ||
| tenantId := c.legacyClient.TenantId() | ||
| c.streams = features.NewStreamsClient(c.legacyClient.Streams(), c.legacyClient.API(), tenantId) |
There was a problem hiding this comment.
Shared publishes can lose order
Two worker goroutines calling Streams() for the first time can create different StreamsClient instances. Each instance has its own queues and producer IDs, so publishes to the same topic can run independently and arrive out of order. Create the shared client during setup or guard its creation with sync.Once.
| ## a failed upload fails only its own publish, as the rest never depended on it | ||
| try: | ||
| ref = await p.uploaded | ||
| except Exception as e: | ||
| p.settle(e) | ||
| continue | ||
|
|
||
| entries.append(streams_protos.StreamPublishEntry(payload_ref=ref)) | ||
| sent.append(p) | ||
|
|
||
| if entries: | ||
| await self._send_entries(sent, entries) |
There was a problem hiding this comment.
Cancelled messages still get published
_send_batch does not check cancellation again after waiting for uploads. If a caller cancels while an upload in the selected batch is running, the drain still sends that message when the upload finishes. This breaks the documented promise to drop publishes cancelled before their request goes out. Remove cancelled entries immediately before sending, including entries collected before a later upload wait.
| # an upload's ref, which a batch carries in place of its payload, rounded up | ||
| PAYLOAD_REF_BYTES = 256 | ||
|
|
||
| Config = Struct.new(:payload_request_attempts, :resubscribe_min_delay_seconds, :resubscribe_max_delay_seconds) do |
There was a problem hiding this comment.
Config lacks keyword_init: true, but the gem supports Ruby 3.1. On that version, the bare super passes the keyword arguments as one positional hash. payload_request_attempts becomes that hash, and the delay fields remain nil. Reconnects then fail when calculating a delay, and failed payload requests cannot retry. Add keyword_init: true or pass the three values positionally.
| Config = Struct.new(:payload_request_attempts, :resubscribe_min_delay_seconds, :resubscribe_max_delay_seconds) do | |
| Config = Struct.new(:payload_request_attempts, :resubscribe_min_delay_seconds, :resubscribe_max_delay_seconds, keyword_init: true) do |
| deadline = Time.now + 10 | ||
|
|
||
| HATCHET.streams.subscribe(topic) do |event| | ||
| puts event.payload | ||
| break if Time.now > deadline |
There was a problem hiding this comment.
Quiet reads ignore the deadline
stop_reading checks its ten-second deadline only after receiving a message. On a quiet topic, it keeps waiting until another message arrives or the server closes the idle stream. Someone copying this example will not get the advertised timeout. Use a timeout that interrupts the blocked read instead of checking time inside the block.
| hatchet "github.com/hatchet-dev/hatchet/sdks/go" | ||
| ) | ||
|
|
||
| type ProgressInput struct { |
There was a problem hiding this comment.
Exported examples lack required comments
The new exported Go types and functions have no godoc comments. The repository requires comments on exported types and functions, so this must be fixed before merging. Add them to ProgressInput, ProgressTask, and the other exported declarations in the shared example, then regenerate its mirror.
Context Used: Comment and logging conventions for the Hatchet codebase (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def events( | ||
| self, topic: str, namespace: str = "", cursor: str | None = None | ||
| ) -> AsyncGenerator[StreamEvent, None]: | ||
| """ | ||
| An alias of `subscribe`, which replaces it; `events` will be removed in a future release. | ||
|
|
||
| :param topic: The topic to read from. | ||
| :param namespace: The topic's namespace; the empty namespace by default. | ||
| :param cursor: A cursor read off a previous event, to resume right after it. | ||
|
|
||
| :return: An async iterator of the topic's events. | ||
| """ | ||
| return self.subscribe(topic, namespace, cursor) |
There was a problem hiding this comment.
prob don't need this since it was never released
There was a problem hiding this comment.
true lol, forgot about that
| payload = ( | ||
| await asyncio.to_thread( | ||
| self._fetch_payload, entry.payload_ref | ||
| ) |
There was a problem hiding this comment.
ideally we'd be able to do this as native-async without needing to_thread. just worried this is going to cause blocked event loops really easily
There was a problem hiding this comment.
we have to do this because the REST API isn't asynchronous :/ unless I'm missing something
There was a problem hiding this comment.
argh alright - is this just for large payloads?
There was a problem hiding this comment.
yep, <4mb payloads go through gRPC path
Description
Fixes # (issue)
Type of change
Checklist
Changes have been:
🤖 AI Disclosure