search

treeform / windy

Windowing library for Nim using OS native APIs.
MIT License
115 stars 16 forks source link

add windyNoHttp define to disable http / async polling #109

Closed elcritch closed 12 months ago

elcritch commented 1 year ago

The async polling is causing crashes when I use it with a regular asyncdispatcher. This PR adds a flag to disable the built-in http polling.

guzba commented 1 year ago

Hi and thanks for the PR, however I do not think this is quite how I'd want to sovle this as-is.

Currently, Mac and Windows do not use Nim async at all unless windyUseStdHttp is set, so there should be no issue there.

Linux should not have any HTTP support at all unless windyUseStdHttp is enabled (since there is no OS support), which would remove the need for windyNoHttp.

This avoids adding another -d that does not do what it says on Windows and Mac.

Also, I am curious about the crashes when using a regular asyncdispatcher. It will be hard to prevent regressions on this without some way of reproducing the issue. Is this possible?

guzba commented 1 year ago

Edit: a I never did the Mac HTTP it appears. Should also be behind windyUseStdHttp then.

elcritch commented 1 year ago

Hi and thanks for the PR, however I do not think this is quite how I'd want to sovle this as-is.

Sure, more of a suggestion and trying to follow what you had. I also though of adding a "poll = true" flag to the proc.

Linux should not have any HTTP support at all unless windyUseStdHttp is enabled (since there is no OS support), which would remove the need for windyNoHttp.

I actually didn't quite follow that. ;) Doesn't Linux have std/http?

Also, I am curious about the crashes when using a regular asyncdispatcher. It will be hard to prevent regressions on this without some way of reproducing the issue. Is this possible?

That might be a bit tricky since it's somewhat random based. Sometimes it'd crash immediately, othertimes it would take a bit.

It looked like some kind of race condition with setting the global dispatcher. In theory that should be fine, but I don't know if there's other threads doing things or whatnot, or other global state.

guzba commented 1 year ago

Ah yeah to clarify I just meant using one flag to turn on or off std/http implementation. Something like https://github.com/treeform/windy/commit/827c2b09b7eef08d54026a12df1e2c10a36a9357

This means it is off unless explicitly enabled, and when enabled one should be mindful of the implications of using other async since this bug sounds a bit heisenbug.

elcritch commented 1 year ago

Ah yeah to clarify I just meant using one flag to turn on or off std/http implementation.

That makes sense. Did you decide on the flag / behavior for enabling and disabling on different platforms?

This means it is off unless explicitly enabled, and when enabled one should be mindful of the implications of using other async since this bug sounds a bit heisenbug.

Yah that's right, you can use it but need to beware of the heisenbug with async dispatch. I actually moved away from async for other reasons.

BTW, I was lazy and dumped changes to fix 2.0 compatibility in this branch too. It may not be the approach you'd take, but there's some compilation issues using _ as a variable name in 2.0. It's fine as a parameter, but you can't pass it to a proc:

proc objcFoo(_: arg) =
  foo_impl(_) # this blows up