Not quite understanding OnSession (program crashes)

[patrickjane]

I am trying to use this library, however I am having a bit of a hard time understanding the concepts. First of all, the examples are outdated. https://github.com/tmaxmax/go-sse/blob/master/cmd/complex/main.go#L36 seems to use deprecated APIs, and not the APIs advised here: https://pkg.go.dev/github.com/tmaxmax/go-sse#Server

Then, I want to publish an initial message to every client connecting, and I thought the place for this would be OnSession, like this:

	sseServer := &sse.Server{
		OnSession: func(session *sse.Session) (sse.Subscription, bool) {
			msg := &sse.Message{}
			msg.AppendData("welcome")

			session.Send(msg)
			session.Flush()
			return sse.Subscription{Topics: []string{sse.DefaultTopic}}, true
		},
	}

	go func() {
		for {
			ev := &sse.Message{}
			ev.AppendData("Hello world")

			sseServer.Publish(ev)

			time.Sleep(5 * time.Second)
		}
	}()

While I do get the initial message just fine, the first attempt to call sseServer.Publish(ev) crashes the program:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x20 pc=0x10290dae8]

goroutine 8 [running]:
github.com/tmaxmax/go-sse.(*Joe).start(0x14000154f00, {0x102bc9c30, 0x103139f40})
	/Users/myuser/go/pkg/mod/github.com/tmaxmax/[email protected]/joe.go:227 +0x408
created by github.com/tmaxmax/go-sse.(*Joe).init.func1 in goroutine 27
	/Users/myuser/go/pkg/mod/github.com/tmaxmax/[email protected]/joe.go:306 +0x1f8

If I remove the OnSession parameter, the sseServer.Publish(ev) will work just fine. So I am not sure how I would set up OnSession correctly to avoid the crash. In any case, the library should have proper error handling here.

I don't want to use subscriptions for typed messages, in fact, I only need to send untyped messages. So I actually don't need all the topic & subscription logic. How do I have to use your library?

[patrickjane]

Okay, I just found out I need to set the Client property to session for the Subscription returned from OnSession (LastEventID too).

I was confused because the API in https://github.com/tmaxmax/go-sse/blob/master/server.go didn't seem to match what I was seeing, but I found out that the current latest version (0.10.0) differs (https://github.com/tmaxmax/go-sse/blob/v0.10.0/server.go).

This leads me to the question if this library was actually stable enough to use it in production? The API still seems to change a lot as it seems.

[patrickjane]

Answering my own question here regarding stability, since I just encountered #50:

• the library seems to contain all the functionality I need
• the API seems not to be stable
• the error handling seems to be not properly implemented, as I got 2 different crashes, where I would expect error returns and the like
• in my opinion libraries like this one must not crash at all costs even on user error (e.g. wrong parameters used, as above), with the exception of the initialization phase of the program / library init

As a result, I can, unfortunately, currently not use this library. Since there is really no good alternatives, I am back to implementing SSE on my own, as I have already done several times. I was hoping this library could make the implementation a bit better, and not homegrown, however to me it currently does not seem stable enough. Fair enough tho the version number is 0.x.x, so this is probably to be expected.

[tmaxmax]

Hi there and thank you again for the issues you've opened.

go-sse is in the middle of a release cycle right now which will change the sse.Server.OnSessionAPI – this is why the examples and documentation from the version v0.10.0 do not match with what is currently on master. What you see on master will be the new API, so the current one that you use in the example will be outdated.

The change the next version brings is meant precisely to eliminate the confusion sse.Server.OnSession brings in its current form – one cannot actually yet use the provided sse.Session inside the OnSession callback. Furthermore, sse.Session is not thread safe, as it is meant to be used solely as a building block for providers, which are supposed to synchronize access to each sse.Session object themselves. I'll tag it soon but in the meantime, to avoid having to adjust your code to the new API, you can use the latest commit in your code:

$ go get github.com/tmaxmax/go-sse@master

This will give you the new API. Code on master is always complete, compiles and is tested, so it is safe to use.

I don't want to use subscriptions for typed messages, in fact, I only need to send untyped messages.

I don't exactly understand what you mean here when you say "typed"/"untyped". Do you mean the sse.Message.Type field, which corresponds to the event field from the SSE protocol? Topics and event types are not linked in any way – topics are used to be able to host multiple event streams from the servers – for example, you might have an application which host two topics, usa and europe, on which events of the same types (say, event: news and event: info) but with content relevant only to the topic's region are published.

If you mean that you don't need topics in this manner but instead each user has an individual, personalized event stream, sending messages to individual connections has been discussed in #36. I've provided a solution there which didn't seem fit for the other person asking but it might work for you. I've yet to thoroughly look into this, though I believe that what I've proposed there should work, albeit with a less intuitive usage.

I want to publish an initial message to every client connecting

Sending a welcome message to connections is addressed in #31. You can expand on that using ideas from #36 to send a personalized message to each user.

To answer your other concerns:

the API seems not to be stable

This is true. There is no v1 yet so every minor version bump brings in breaking changes. The library strictly follows semantic versioning, so if only the patch version is bumped then there aren't any breaking changes. If I do breaking changes, I usually change one single component in a version, so that users of the library don't have to face too many changes at once.

the error handling seems to be not properly implemented, as I got 2 different crashes, where I would expect error returns and the like

As of now on the master branch, the library explicitly calls panic in one single place in the library, when using the library's SSE client. All other error scenarios that I know of should be properly handled or propagated, and panics – especially in the provider, which is supposed to be long-running – are caught and handled gracefully. If you've encountered crashes it's either because of misusage or undiscovered bugs, in order to provide assitance or find a fix seeing the actual code would be very helpful.

in my opinion libraries like this one must not crash at all costs even on user error

When it comes to user/programmer errors, it's close to impossible to predict all mistaken usages one might come up with. There are various panics which, on wrong usage, occur outside the library's execution scope, so it would be impossible to catch those panics.

What the library can do is implement intuitive and well documented APIs which effectively do not permit misusage, and where it can't do that to properly validate the invariants which are in the library's own scope of execution. I agree that in your situation above it would've been nice to receive some message to indicate that no Client was provided instead of a panic – but the updated API doesn't even allow the misusage, as the user doesn't have to create the sse.Subscription object anymore.

Hopefully my standpoint makes sense. What is important is that in the development of go-sse I try to the best of my ability to in no way overlook any facet of error handling and guarantee functional stability of the code, i.e. that it works. API instability does not have to correlate to bugs or incomplete code, and while the former is not yet a priority for go-sse, having code that works is obviously essential. There are also quite a few v0 libraries which can be used in production without issue.

Again, if you think you've found a bug, a reproducible example helps tremendously.

the library seems to contain all the functionality I need

This is awesome! I should be able then to further assist you on your use case. Helping with concrete code in #50 and maybe a short description of what you're actually trying to achieve should give me enough input to propose a solution or find a fix for you.

I'm very eager to help you further or fix anything wrong you've discovered and I'm sure we can find a way to neatly integrate go-sse into your application. Thank you again for your interest in go-sse and looking forward to further collaborate together on this issue!

[patrickjane]

I don't want to use subscriptions for typed messages, in fact, I only need to send untyped messages.

I don't exactly understand what you mean here when you say "typed"/"untyped". Do you mean the sse.Message.Type field, which corresponds to the event field from the SSE protocol? Topics and event types are not linked in any way – topics are used to be able to host multiple event streams from the servers – for example, you might have an application which host two topics, usa and europe, on which events of the same types (say, event: news and event: info) but with content relevant only to the topic's region are published.

Yeah well this was a bit misleading. Yes I mean messages without event type, and only data.

in my opinion libraries like this one must not crash at all costs even on user error

When it comes to user/programmer errors, it's close to impossible to predict all mistaken usages one might come up with. There are various panics which, on wrong usage, occur outside the library's execution scope, so it would be impossible to catch those panics.

What the library can do is implement intuitive and well documented APIs which effectively do not permit misusage, and where it can't do that to properly validate the invariants which are in the library's own scope of execution. I agree that in your situation above it would've been nice to receive some message to indicate that no Client was provided instead of a panic – but the updated API doesn't even allow the misusage, as the user doesn't have to create the sse.Subscription object anymore.

I have to partly disagree. For one, it should not panic. While this is also done in a few other libraries - the more usual case seems to be that libraries don't panic during runtime in go. This might be a bit subject to personal preference - I prefer non-panicking / non throwing libraries in general - if it must panic for whatever reason, it must be clearly stated in the API docs, so that users can handle this case properly. But still, to me, panic + recover is not a common usage scenario in Go libraries.

But I was not talking about explicit panic()s, but rather crashes. E.g. in one of my reported crashes a nil pointer was accessed, and the program crashed.
Of course its tedious and a lot of work, but - in my opinion - libraries, especially public libraries, should make sure as good as they can that such crashes don't happen, but the (potential user-)error is reported to the called in a graceful manner.

Again, if you think you've found a bug, a reproducible example helps tremendously.

Well, as stated, create a server with 2 endpoints, one receives the SSE-stream, the other triggers Server.Publish and then spam the second endpoint.

[tmaxmax]

public libraries, should make sure as good as they can that such crashes don't happen, but the (potential user-)error is reported to the called in a graceful manner.

Point taken, that one would have been preventable with a bit of validation code. I'll keep this in mind.

create a server with 2 endpoints, one receives the SSE-stream, the other triggers Server.Publish and then spam the second endpoint.

So I've created the following server:

package main

import (
	"log/slog"
	"net/http"
	"time"

	"github.com/tmaxmax/go-sse"
)

func main() {
	s := &sse.Server{
		Logger: func(*http.Request) *slog.Logger {
			return slog.Default()
		},
	}

	mux := http.NewServeMux()
	mux.Handle("GET /events", s)
	mux.HandleFunc("POST /update", func(http.ResponseWriter, *http.Request) {
		m := &sse.Message{}
		m.AppendData(time.Now().String())
		_ = s.Publish(m)
	})

	if err := http.ListenAndServe(":8000", mux); err != nil {
		slog.Default().Error("listen and serve", "err", err)
	}
}

which, excluding the logging code, is the most basic version that satisfies your description, and the following client:

package main

import (
	"fmt"
	"net/http"
	"os"

	"github.com/tmaxmax/go-sse"
)

func main() {
	r, _ := http.NewRequest(http.MethodGet, "http://localhost:8000/events", http.NoBody)
	conn := sse.NewConnection(r)

	conn.SubscribeMessages(func(event sse.Event) {
		fmt.Printf("%s\n\n", event.Data)
	})

	if err := conn.Connect(); err != nil {
		fmt.Fprintln(os.Stderr, err)
	}
}

(it's basically the helloworld_client example with a different endpoint).

From two distinct command lines, I'm executing the following command in a loop:

$ curl -X POST http://localhost:8000/update

with a sleep 0.01 in one CLI and sleep 0.03 in another CLI in between.

I've connected three distinct clients to the server. After six minutes of running, the program still behaves normally and events are published and received without issue. I've also sometimes disconnected one client to see whether the server breaks but it is well behaved. I've also removed the sleep command and added some ~10 other clients, plus a browser EventSource client. It still worked properly. So I'm not able to reproduce your crash

I'm sure that my code and testing method is different from yours. If you could share your code and precisely state the steps to reproduce it (i.e. what exactly is the server setup code, how did you connect to the server, how did you spam the endpoint) to see what happens there it would be tremendously helpful.