Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Caching data response for complete event parsing #21

Merged
merged 8 commits into from
Nov 30, 2024

Conversation

komkovla
Copy link
Contributor

Buffer

Add buffer in parser to store parts of the messages that are not complete, as mentioned in #19

Now parser would store received data parse full messages and retain remaining of the message to parse with next chunk

If the connection closes and buffer is not empty events are not processed, this behaviour is according to the SSE documentation

If the file ends in the middle of an event, before the final empty line, the incomplete event is not dispatched.

Thread safety

To provide thread-safe access to buffer, parser is isolated to an actor.

Test Cases Adjustments:

Typo

Fixed a typo in the sessionDelegateTask variable name. [1] [2] [3] [4]

Vladislav Komkov added 5 commits November 21, 2024 13:46
This solves issue processing chunk of data without full event data
Event is completed only when double new line is present, otherwise we don't process partial event
Insert in buffer only not complete messages reducing amount of data needed to store
Copy link
Owner

@Recouse Recouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

I left some comments. Apart making ServerEventParser an actor, everything else seems fine.

@@ -9,12 +9,13 @@
import Foundation

public protocol EventParser: Sendable {
func parse(_ data: Data) -> [EVEvent]
func parse(_ data: Data) async -> [EVEvent]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function be marked as async?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I allowed async function in EventParser protocol to allow conformance for an actor that run asynchronous

}

/// ``ServerEventParser`` is used to parse text data into ``ServerEvent``.
public struct ServerEventParser: EventParser {
let mode: EventSource.Mode
actor ServerEventParser: EventParser {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of making this an actor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServerEventParser has a mutable property buffer, so its better to use reference type such as class, then to provide exclusive access to modifying buffer property and ensuring data-race safety I make ServerEventParser an actor, which ensures only one thread at a time could access this data. But I am still new to those concepts in Swift so if you have a better solution in mind please share 🙂

Sources/EventSource/EventParser.swift Show resolved Hide resolved
Co-authored-by: Firdavs Khaydarov <[email protected]>
Copy link
Owner

@Recouse Recouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since EventParser and all its implementations are Sendable and thread safe, there's no need to make ServerEventParser an actor, and therefore the parse(_:) function does not need to be async.

I've made some changes. You'll also need to update the tests based on the compiler's suggestions. Once that's done, I can merge it

@@ -9,12 +9,13 @@
import Foundation

public protocol EventParser: Sendable {
func parse(_ data: Data) -> [EVEvent]
func parse(_ data: Data) async -> [EVEvent]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func parse(_ data: Data) async -> [EVEvent]
mutating func parse(_ data: Data) async -> [EVEvent]

} else {
rawMessages = data.split(by: [Self.lf, Self.lf])
}
func parse(_ data: Data) -> [EVEvent] {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func parse(_ data: Data) -> [EVEvent] {
mutating func parse(_ data: Data) -> [EVEvent] {

} else {
rawMessages = data.split(by: [Self.lf, Self.lf])
}
func parse(_ data: Data) -> [EVEvent] {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func parse(_ data: Data) -> [EVEvent] {
mutating func parse(_ data: Data) -> [EVEvent] {

@@ -146,19 +146,19 @@ public extension EventSource {
case let .didReceiveResponse(response, completionHandler):
handleSessionResponse(response, completionHandler: completionHandler)
case let .didReceiveData(data):
parseMessages(from: data)
await parseMessages(from: data)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await parseMessages(from: data)
parseMessages(from: data)

@@ -239,7 +239,7 @@ public extension EventSource {
cancel()
}

private func parseMessages(from data: Data) {
private func parseMessages(from data: Data) async {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private func parseMessages(from data: Data) async {
private func parseMessages(from data: Data) {

@@ -248,7 +248,7 @@ public extension EventSource {
return
}

let events = eventParser.parse(data)
let events = await eventParser.parse(data)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let events = await eventParser.parse(data)
let events = eventParser.parse(data)

@Recouse
Copy link
Owner

Recouse commented Nov 30, 2024

Thank you for the contribution, this is a great improvement to the package!

@Recouse Recouse merged commit bc6cf4b into Recouse:main Nov 30, 2024
3 checks passed
@komkovla komkovla deleted the bug/caching-responce branch November 30, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants