search

niv / websocket.nim

websockets for nim
http://niv.github.io/websocket.nim/docs/0.1.1/websocket.html
Other
101 stars 25 forks source link

fix gc unsafe. #16

Closed enthus1ast closed 6 years ago

niv commented 6 years ago

Wouldn't this break? The global map is global because it is needed between readData calls.

What about making it thread-local instead (though that will potentially break too for multithreaded httpds that shard out request handlers)?

Maybe this needs a bigger API change and move responsibility for this elsewhere.

enthus1ast commented 6 years ago

Yep, we need to get rid of the global.

dom96 commented 6 years ago

Nim needs a way for libraries to initialise thread-local vars.

CC @Araq

Araq commented 6 years ago

We had introduced a mechanism for that but it encouraged races everywhere so we removed it again.

niv commented 6 years ago

Well, re: the patch: We can make it a thread-local and init it on first usage, but then it'll break for server designs where the same connection can hop between threads. It would definitely have to be documented.

Moving it into the function scope seems wrong to me, as pings will never be answered (the table will always be empty for every new request unpack).

dom96 commented 6 years ago

@Araq how did it encourage races? Maybe the mechanism's implementation was wrong? :P

enthus1ast commented 6 years ago

@niv mabye something like a websocketContext ?

Araq commented 6 years ago

Why not use a SharedTable instead? Needs protect and dispose for the Future but it's possible.