kenichi / angelo

Sinatra-like DSL for Reel that supports WebSockets and SSE
Other
303 stars 23 forks source link

fix sse on_close. invoke callback when socket is about to close. #74

Closed respire closed 3 years ago

respire commented 7 years ago

Description: after each request, server will push responder into an array. if connection is alive for a long time, responders and objects they refer to won't be recycled by GC, which may result in out of memory issue. After deep diving Readme and source code, I guess on_close is a feature part of EventSource. websocket or normal http request is not supporting such feature (they don't have helper method from Angelo::Base to set the callback).

Solution: only execute on_close block for EventSource since there's no need for normal HTTP requests or websocket ones.

kenichi commented 7 years ago

hi @respire thanks for this. i have looked further into this and agree there is a problem, holding onto every responder for the duration of the connection, regardless of on_close being set or not.

i'm starting to think the whole concept might need revisiting - especially with respect to after blocks. websockets have their own on_close blocks, this comes for free via the object reel hands off.

anyways, sorry for the lag, but i wanted to respond and let you know i'm thinking more about this and not just ignoring it. 😄

respire commented 7 years ago

hi @kenichi , I agree we need the revisiting.

at my first glance at Readme, the basic use case for ES#on_close is to delete instance from Base#sses. but i think it should be done automatically by angelo/reel, not by users. Users tend to forget this step.

In addition, since the socket has already been closed (especially when browser's tab is closed), we cannot send more bytes or do anything else on that socket. so I really not sure which case a framework user may want on_close hook.

but I still think these are not the reasons why we should remove on_close. they're just the points keep confusing me.