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

Go error handling #8

Closed
tapir opened this Issue May 29, 2013 · 32 comments

Comments

Projects
None yet
5 participants
@tapir
Member

tapir commented May 29, 2013

Is it better to use Go's error functionality instead of returning nil pointers?

@mewmew

This comment has been minimized.

Show comment
Hide comment
@mewmew

mewmew Jun 2, 2013

Contributor

I think this would make the API more idiomatic.

Below are the update function definitions of the affected API.

func GetMonitors() ([]*Monitor, error)
func GetPrimaryMonitor() (*Monitor, error)
func (m *Monitor) GetVideoMode() (*VideoMode, error)
func (m *Monitor) GetVideoModes() ([]*VideoMode, error)
func CreateWindow(width, height int, title string, monitor *Monitor, share *Window) (*Window, error)
func GetCurrentContext() (*Window, error)
func (w *Window) GetMonitor() (*Monitor, error)
Contributor

mewmew commented Jun 2, 2013

I think this would make the API more idiomatic.

Below are the update function definitions of the affected API.

func GetMonitors() ([]*Monitor, error)
func GetPrimaryMonitor() (*Monitor, error)
func (m *Monitor) GetVideoMode() (*VideoMode, error)
func (m *Monitor) GetVideoModes() ([]*VideoMode, error)
func CreateWindow(width, height int, title string, monitor *Monitor, share *Window) (*Window, error)
func GetCurrentContext() (*Window, error)
func (w *Window) GetMonitor() (*Monitor, error)
@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Jun 2, 2013

Member

I think even Init() can return an error instead of a bool?

Member

tapir commented Jun 2, 2013

I think even Init() can return an error instead of a bool?

@mewmew

This comment has been minimized.

Show comment
Hide comment
@mewmew

mewmew Jun 3, 2013

Contributor

I agree that a failure should be indicated with an error value. However, since Init (and most other functions) can fail for multiple reasons it would be beneficial if we could provide any additional information.

For instance Init can fail with any of the following error messages (on X11):

"X11: Failed to open X display"
"X11: Failed to query RandR version"
"X11: The keyboard extension is not available"
"X11: Failed to set detectable key repeat"
"X11: Detectable key repeat is not supported"
...

I think the sole purpose that glfw uses an error callback is because C lacks multiple return values.

Given that glfw requires a single thread of execution (see note below) it should be possible to implement an internal error callback function and propagate the error information back to the appropriate function so it can return a true error message.

Note: The behavior may have changed in glfw 3.0, were a single thread for all GLFW calls is no longer the case. This has to be verified.

If we choose this approach it should no longer be possible to set the error callback to a custom function, and there would be no reason for it either since all the error information would be available at each call point. SetErrorCallback would no longer be exported.

What are your thoughts on the matter? This would arguably change the semantics of the glfw library, but doing so would provide an API that utilizes Go's support for multiple return values. To me this would feel more idiomatic.

Contributor

mewmew commented Jun 3, 2013

I agree that a failure should be indicated with an error value. However, since Init (and most other functions) can fail for multiple reasons it would be beneficial if we could provide any additional information.

For instance Init can fail with any of the following error messages (on X11):

"X11: Failed to open X display"
"X11: Failed to query RandR version"
"X11: The keyboard extension is not available"
"X11: Failed to set detectable key repeat"
"X11: Detectable key repeat is not supported"
...

I think the sole purpose that glfw uses an error callback is because C lacks multiple return values.

Given that glfw requires a single thread of execution (see note below) it should be possible to implement an internal error callback function and propagate the error information back to the appropriate function so it can return a true error message.

Note: The behavior may have changed in glfw 3.0, were a single thread for all GLFW calls is no longer the case. This has to be verified.

If we choose this approach it should no longer be possible to set the error callback to a custom function, and there would be no reason for it either since all the error information would be available at each call point. SetErrorCallback would no longer be exported.

What are your thoughts on the matter? This would arguably change the semantics of the glfw library, but doing so would provide an API that utilizes Go's support for multiple return values. To me this would feel more idiomatic.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Jun 3, 2013

Member

The only reason I did not implement this yet is because I was reluctant to change the glfw semantics but as you noted it will make a more idiomatic wrapper. I think this would be the way to go.

Member

tapir commented Jun 3, 2013

The only reason I did not implement this yet is because I was reluctant to change the glfw semantics but as you noted it will make a more idiomatic wrapper. I think this would be the way to go.

@mewmew

This comment has been minimized.

Show comment
Hide comment
@mewmew

mewmew Jun 3, 2013

Contributor

I also think it is the way forward.

Before we can provide an implementation we have to figure out how it would work in the context of multiple windows which may run in multiple OS threads.

From http://www.glfw.org/docs/3.0/context.html:

Note that a context can only be current for a single thread at a time, and a thread can only have a single context at a time.

This implies that it could be possible to run one dedicated thread per window and call MakeContextCurrent on each window in each thread. In that case we would have to register a custom error callback for each thread. Do you have the same perception or have I misunderstood how glfw operates?

Contributor

mewmew commented Jun 3, 2013

I also think it is the way forward.

Before we can provide an implementation we have to figure out how it would work in the context of multiple windows which may run in multiple OS threads.

From http://www.glfw.org/docs/3.0/context.html:

Note that a context can only be current for a single thread at a time, and a thread can only have a single context at a time.

This implies that it could be possible to run one dedicated thread per window and call MakeContextCurrent on each window in each thread. In that case we would have to register a custom error callback for each thread. Do you have the same perception or have I misunderstood how glfw operates?

tapir added a commit that referenced this issue Jun 4, 2013

tapir added a commit that referenced this issue Jun 4, 2013

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Jun 17, 2013

Member

I fiddled with this but could not come up with a good method. I will close it if you don't want to work on it.

Member

tapir commented Jun 17, 2013

I fiddled with this but could not come up with a good method. I will close it if you don't want to work on it.

@mewmew

This comment has been minimized.

Show comment
Hide comment
@mewmew

mewmew Jun 20, 2013

Contributor

Is it possible to mark the issue as thinking similar to how Go does with some issues? That way you don't have to close it. I think this would drastically improve the API, but it has to be implemented in a way that isn't trivial to envision.

I would rather leave this open and continue to think about it, than close it and probably forget about it.

Either way, I'm not working actively on it - but I would like to come back to this issue in the future when we know how to solve it. I would happily implement a solution for it if we just figure out how that should be done.

Contributor

mewmew commented Jun 20, 2013

Is it possible to mark the issue as thinking similar to how Go does with some issues? That way you don't have to close it. I think this would drastically improve the API, but it has to be implemented in a way that isn't trivial to envision.

I would rather leave this open and continue to think about it, than close it and probably forget about it.

Either way, I'm not working actively on it - but I would like to come back to this issue in the future when we know how to solve it. I would happily implement a solution for it if we just figure out how that should be done.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Jun 20, 2013

Member

Done.

When I have time I'll look into glfw source to figure out how exactly callbacks work between different threads. That might help.

Member

tapir commented Jun 20, 2013

Done.

When I have time I'll look into glfw source to figure out how exactly callbacks work between different threads. That might help.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 25, 2013

Member

What part(s) of this issue remains open?

Member

dmitshur commented Sep 25, 2013

What part(s) of this issue remains open?

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Sep 25, 2013

Member

all of it.
if you have an idea how we can return go errors from needed functions using the internal glfw error callback mechanism that would be great.

Member

tapir commented Sep 25, 2013

all of it.
if you have an idea how we can return go errors from needed functions using the internal glfw error callback mechanism that would be great.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 25, 2013

Member

All of it? It looks to me like #29 merged most of these changes in.

E.g., http://godoc.org/github.com/go-gl/glfw3#GetMonitors now returns ([]*Monitor, error).

Member

dmitshur commented Sep 25, 2013

All of it? It looks to me like #29 merged most of these changes in.

E.g., http://godoc.org/github.com/go-gl/glfw3#GetMonitors now returns ([]*Monitor, error).

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Sep 25, 2013

Member

This issue is about the functions that return a "bool" like glfw.Init(). That's when the error callback of glfw kicks in.
In #29 C versions of those functions return a "null" when there is an error.

edit: My mistake. The issue was originally opened for what you stated. I should clarify.

Member

tapir commented Sep 25, 2013

This issue is about the functions that return a "bool" like glfw.Init(). That's when the error callback of glfw kicks in.
In #29 C versions of those functions return a "null" when there is an error.

edit: My mistake. The issue was originally opened for what you stated. I should clarify.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 25, 2013

Member

Ok, so the open issue is related to things that still trigger the error callback (what are they all, exactly?). That's what I suspected, I just wanted to confirm.

I remember the GetClipboardString() used to return (string) only and call the error callback on error, which made it very difficult to use. Now that it returns (string, error), it's a pleasure to use because the error can be easily handled in context.

Member

dmitshur commented Sep 25, 2013

Ok, so the open issue is related to things that still trigger the error callback (what are they all, exactly?). That's what I suspected, I just wanted to confirm.

I remember the GetClipboardString() used to return (string) only and call the error callback on error, which made it very difficult to use. Now that it returns (string, error), it's a pleasure to use because the error can be easily handled in context.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Sep 25, 2013

Member

Ok, let me clarify this because turns out I forgot about this issue.

I think the only function left that doesn't return an error yet invokes the error callback is glfw.Init().

But all the commits related to this issue are only workarounds because they don't return the "error code" or "description" like an error callback would. This is how I return errors:

func (w *Window) GetClipboardString() (string, error) {
    cs := C.glfwGetClipboardString(w.data)
    if cs == nil {
        return "", errors.New("Can't get clipboard string.")
    }

    return C.GoString(cs), nil
}

So I believe this issue still exists. Ideally we should be able to return the "error code" and "description" coming from the error callback.

Member

tapir commented Sep 25, 2013

Ok, let me clarify this because turns out I forgot about this issue.

I think the only function left that doesn't return an error yet invokes the error callback is glfw.Init().

But all the commits related to this issue are only workarounds because they don't return the "error code" or "description" like an error callback would. This is how I return errors:

func (w *Window) GetClipboardString() (string, error) {
    cs := C.glfwGetClipboardString(w.data)
    if cs == nil {
        return "", errors.New("Can't get clipboard string.")
    }

    return C.GoString(cs), nil
}

So I believe this issue still exists. Ideally we should be able to return the "error code" and "description" coming from the error callback.

tapir added a commit that referenced this issue Nov 26, 2013

@mewmew

This comment has been minimized.

Show comment
Hide comment
@mewmew

mewmew May 22, 2014

Contributor

As stated by @tapir this issue was fixed by rev 2d75321. Does anything remain to be done or should the issue be closed?

Btw, thanks @tapir for a clean fix to a non-trivial problem :)

Ah.. Just noticed that the commit was part of the development branch. Sorry for the noise.

Contributor

mewmew commented May 22, 2014

As stated by @tapir this issue was fixed by rev 2d75321. Does anything remain to be done or should the issue be closed?

Btw, thanks @tapir for a clean fix to a non-trivial problem :)

Ah.. Just noticed that the commit was part of the development branch. Sorry for the noise.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir May 23, 2014

Member

You're welcome! I will merge the devel branch once GLFW 3.1 is out.

Member

tapir commented May 23, 2014

You're welcome! I will merge the devel branch once GLFW 3.1 is out.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 28, 2014

Member

Testing the 3.1 development branch I found a serious problem with the error handling: deadlocking.

  • Some GLFW functions do not really advertise generating errors, but actually do.
  • When these functions generate errors, the error handler tries to send them over the channel and nobody is listening for it (causing a deadlock).

For instance, at least on Linux/X11, func WindowHint(target Hint, hint int) will generate an InvalidEnum error in some cases (I just ran into one), causing a deadlock.

Looking at GLFW's source code shows it is probably a deeper problem, for instance:

glfwRestoreWindow at window.c:558 internally calls _glfwPlatformRestoreWindow at x11_window.c:1348 which can generate GLFW_API_UNAVAILABLE if the WM does not support EWMH.

A careful look at the source code and scanning using grep shows us which functions will cause deadlocks right now:

  • glfwJoystickPresent can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetJoystickAxes can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetJoystickButtons can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetJoystickName can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwSetGamma can generate GLFW_INVALID_VALUE, causing a deadlock.
  • glfwSetInputMode can generate several errors, causing a deadlock.
  • glfwGetInputMode can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetKey can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetMouseButton can generate GLFW_INVALID_ENUM, causing a deadlock.

And there is a whole slew of them that are platform-specific in random little caveat-like places. Here is a list for just windows:

  • glfwSetWindowTitle can generate GLFW_PLATFORM_ERROR, causing a deadlock.
  • glfwSetWindowSize calls _glfwPlatformSetWindowSize calls _glfwSetVideoMode can generate GLFW_PLATFORM_ERROR, causing a deadlock.
  • glfwSetGammaRamp calls can generate GLFW_PLATFORM_ERROR, causing a deadlock (but only if (ramp->size != 256)).
  • glfwSetClipboardString can generate GLFW_PLATFORM_ERROR.
  • glfwGetClipboardString can generate GLFW_FORMAT_UNAVAILABLE, and GLFW_PLATFORM_ERROR.

The list of platform-specific errors generated in small caveat-like areas goes on and on.. run grep to see them all:
grep -r _glfwInputError path/to/GLFW/src

It isn't very compatible with future changes to GLFW either IMO.

Thoughts on what to do?

Member

slimsag commented Aug 28, 2014

Testing the 3.1 development branch I found a serious problem with the error handling: deadlocking.

  • Some GLFW functions do not really advertise generating errors, but actually do.
  • When these functions generate errors, the error handler tries to send them over the channel and nobody is listening for it (causing a deadlock).

For instance, at least on Linux/X11, func WindowHint(target Hint, hint int) will generate an InvalidEnum error in some cases (I just ran into one), causing a deadlock.

Looking at GLFW's source code shows it is probably a deeper problem, for instance:

glfwRestoreWindow at window.c:558 internally calls _glfwPlatformRestoreWindow at x11_window.c:1348 which can generate GLFW_API_UNAVAILABLE if the WM does not support EWMH.

A careful look at the source code and scanning using grep shows us which functions will cause deadlocks right now:

  • glfwJoystickPresent can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetJoystickAxes can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetJoystickButtons can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetJoystickName can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwSetGamma can generate GLFW_INVALID_VALUE, causing a deadlock.
  • glfwSetInputMode can generate several errors, causing a deadlock.
  • glfwGetInputMode can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetKey can generate GLFW_INVALID_ENUM, causing a deadlock.
  • glfwGetMouseButton can generate GLFW_INVALID_ENUM, causing a deadlock.

And there is a whole slew of them that are platform-specific in random little caveat-like places. Here is a list for just windows:

  • glfwSetWindowTitle can generate GLFW_PLATFORM_ERROR, causing a deadlock.
  • glfwSetWindowSize calls _glfwPlatformSetWindowSize calls _glfwSetVideoMode can generate GLFW_PLATFORM_ERROR, causing a deadlock.
  • glfwSetGammaRamp calls can generate GLFW_PLATFORM_ERROR, causing a deadlock (but only if (ramp->size != 256)).
  • glfwSetClipboardString can generate GLFW_PLATFORM_ERROR.
  • glfwGetClipboardString can generate GLFW_FORMAT_UNAVAILABLE, and GLFW_PLATFORM_ERROR.

The list of platform-specific errors generated in small caveat-like areas goes on and on.. run grep to see them all:
grep -r _glfwInputError path/to/GLFW/src

It isn't very compatible with future changes to GLFW either IMO.

Thoughts on what to do?

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

Can you send an example for the deadlock code?

Member

tapir commented Aug 29, 2014

Can you send an example for the deadlock code?

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

A quick solution for this can be to increase the buffer of the error channel to something that will never be reached so that the channel is never blocked.

Member

tapir commented Aug 29, 2014

A quick solution for this can be to increase the buffer of the error channel to something that will never be reached so that the channel is never blocked.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 29, 2014

Member

Sure thing:

package main

import glfw "github.com/slimsag/glfw3"

func main() {
    err := glfw.Init()
    if err != nil {
        panic(err)
    }

    // Deadlock here! Invalid enum.
    glfw.WindowHint(-23123, 0)
    glfw.WindowHint(-23123, 0)
}

EDIT: hmm. this code might not be correct.
EDIT 2: fixed it. You need two WindowHint() calls for it to fill the channel buffer.

A quick solution for this can be to increase the buffer of the error channel to something that will never be reached.

IMHO a better short-term solution would be some tiny timeout and a print statement:

select{
    case sendThe <- error:
    case time.After(250 * time.Millisecond):
        fmt.Println("GLFW:", error)
}
Member

slimsag commented Aug 29, 2014

Sure thing:

package main

import glfw "github.com/slimsag/glfw3"

func main() {
    err := glfw.Init()
    if err != nil {
        panic(err)
    }

    // Deadlock here! Invalid enum.
    glfw.WindowHint(-23123, 0)
    glfw.WindowHint(-23123, 0)
}

EDIT: hmm. this code might not be correct.
EDIT 2: fixed it. You need two WindowHint() calls for it to fill the channel buffer.

A quick solution for this can be to increase the buffer of the error channel to something that will never be reached.

IMHO a better short-term solution would be some tiny timeout and a print statement:

select{
    case sendThe <- error:
    case time.After(250 * time.Millisecond):
        fmt.Println("GLFW:", error)
}
@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

@slimsag

I get this when I try to run any app with devel_glfw3.1, any ideas?

alt text

For a long term solution,
@elmindreda do you think it's possible to advertise all the functions that can raise an error in documentation? GLFW users would benefit from this also since they would know when they can expect errors.

And I would really like to just return errors when it occurs one, that's the Go way.

Member

tapir commented Aug 29, 2014

@slimsag

I get this when I try to run any app with devel_glfw3.1, any ideas?

alt text

For a long term solution,
@elmindreda do you think it's possible to advertise all the functions that can raise an error in documentation? GLFW users would benefit from this also since they would know when they can expect errors.

And I would really like to just return errors when it occurs one, that's the Go way.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 29, 2014

Member

@tapir

  • 386 or amd64 architecture?
  • Where did you install MinGW from?
  • I can solve the issue tomorrow and submit a PR -- but now I am going to sleep (hint: you probably need to link against libmingw32 for those symbols).

And I would really like to just return errors when it occurs one, that's the Go way.

I agree with this strongly. Sadly from looking over GLFW's source I think that realistically -- any function GLFW can generate an error (or at least in the future).

Member

slimsag commented Aug 29, 2014

@tapir

  • 386 or amd64 architecture?
  • Where did you install MinGW from?
  • I can solve the issue tomorrow and submit a PR -- but now I am going to sleep (hint: you probably need to link against libmingw32 for those symbols).

And I would really like to just return errors when it occurs one, that's the Go way.

I agree with this strongly. Sadly from looking over GLFW's source I think that realistically -- any function GLFW can generate an error (or at least in the future).

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

386 or amd64 architecture?
Where did you install MinGW from?
I can solve the issue tomorrow and submit a PR -- but now I am going to sleep (hint: you probably need to link against libmingw32 for those symbols).

386, go 1.3.1, official mingw installer.
I will try linking against it.

I agree with this strongly. Sadly from looking over GLFW's source I think that realistically -- any function GLFW can generate an error (or at least in the future).

As long as we get well defined docs from mainstream I don't think it's a problem.

IMHO a better short-term solution would be some tiny timeout and a print statement:

Timeout is a good idea though I wonder what would be a safe value?

Member

tapir commented Aug 29, 2014

386 or amd64 architecture?
Where did you install MinGW from?
I can solve the issue tomorrow and submit a PR -- but now I am going to sleep (hint: you probably need to link against libmingw32 for those symbols).

386, go 1.3.1, official mingw installer.
I will try linking against it.

I agree with this strongly. Sadly from looking over GLFW's source I think that realistically -- any function GLFW can generate an error (or at least in the future).

As long as we get well defined docs from mainstream I don't think it's a problem.

IMHO a better short-term solution would be some tiny timeout and a print statement:

Timeout is a good idea though I wonder what would be a safe value?

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 29, 2014

Member

We should probably panic on any of these errors since they represent a programmer fault or an unrecoverable error (e.g. out of memory).

GLFW_NOT_INITIALIZED
GLFW_NO_CURRENT_CONTEXT
GLFW_INVALID_ENUM
GLFW_INVALID_VALUE
GLFW_OUT_OF_MEMORY

That doesn't help for the others, though.

Member

slimsag commented Aug 29, 2014

We should probably panic on any of these errors since they represent a programmer fault or an unrecoverable error (e.g. out of memory).

GLFW_NOT_INITIALIZED
GLFW_NO_CURRENT_CONTEXT
GLFW_INVALID_ENUM
GLFW_INVALID_VALUE
GLFW_OUT_OF_MEMORY

That doesn't help for the others, though.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

I wouldn't assume what user wants. Although it's hard to imagine where these errors can be recoverable, it's just better if the user decides.

I get this when I try to run any app with devel_glfw3.1, any ideas?

Looks like we're missing #include <string.h> somewhere...

Member

tapir commented Aug 29, 2014

I wouldn't assume what user wants. Although it's hard to imagine where these errors can be recoverable, it's just better if the user decides.

I get this when I try to run any app with devel_glfw3.1, any ideas?

Looks like we're missing #include <string.h> somewhere...

@elmindreda

This comment has been minimized.

Show comment
Hide comment
@elmindreda

elmindreda Aug 29, 2014

I am summoned. Unfortunately, I won't have time for a proper read-through of this thread until after the weekend.

@tapir Every GLFW function except for glfwSetErrorCallback, glfwGetVersion and glfwGetVersionString may cause errors.

On the topic of threads, the 3.1 documentation has a section on thread safety.

elmindreda commented Aug 29, 2014

I am summoned. Unfortunately, I won't have time for a proper read-through of this thread until after the weekend.

@tapir Every GLFW function except for glfwSetErrorCallback, glfwGetVersion and glfwGetVersionString may cause errors.

On the topic of threads, the 3.1 documentation has a section on thread safety.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

Then I think all the functions should return an _error_.
This is somewhat a deviation from the GLFW API but this wrapper is supposed to use Go idioms.

I would like to hear your input on this @shurcooL

Member

tapir commented Aug 29, 2014

Then I think all the functions should return an _error_.
This is somewhat a deviation from the GLFW API but this wrapper is supposed to use Go idioms.

I would like to hear your input on this @shurcooL

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 29, 2014

Member

I think either way it is okay. If there is an error setting the window title -- there isn't anything you can do about it. Its like if fmt.Println returns an error -- what will you do?

Making everything return an error sounds good to me, but people are going to ignore them. So then we might want to have a debug flag that causes errors to be print out to stdout IMO.

Member

slimsag commented Aug 29, 2014

I think either way it is okay. If there is an error setting the window title -- there isn't anything you can do about it. Its like if fmt.Println returns an error -- what will you do?

Making everything return an error sounds good to me, but people are going to ignore them. So then we might want to have a debug flag that causes errors to be print out to stdout IMO.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 29, 2014

Member

So I made some progress. Instead of using a timeout I went with select/default case so that the channel becomes non-blocking.

Here are some reads:
https://gobyexample.com/timeouts
https://gobyexample.com/non-blocking-channel-operations

For some reason though, without a closure around select it doesn't work

//export goErrorCB
func goErrorCB(code C.int, desc *C.char) {
    go func() {
        select {
        case lastError <- &GlfwError{ErrorCode(code), C.GoString(desc)}:
        default:
            fmt.Printf("Uncought error: %d -> %s\n", ErrorCode(code), C.GoString(desc))
        }
    }()
}

Anyway, this is not necessarily needed if we're going to return errors for all needing functions but, this should stay there just as a safety precaution for future since GLFW can break this for us without affecting their own API.

Member

tapir commented Aug 29, 2014

So I made some progress. Instead of using a timeout I went with select/default case so that the channel becomes non-blocking.

Here are some reads:
https://gobyexample.com/timeouts
https://gobyexample.com/non-blocking-channel-operations

For some reason though, without a closure around select it doesn't work

//export goErrorCB
func goErrorCB(code C.int, desc *C.char) {
    go func() {
        select {
        case lastError <- &GlfwError{ErrorCode(code), C.GoString(desc)}:
        default:
            fmt.Printf("Uncought error: %d -> %s\n", ErrorCode(code), C.GoString(desc))
        }
    }()
}

Anyway, this is not necessarily needed if we're going to return errors for all needing functions but, this should stay there just as a safety precaution for future since GLFW can break this for us without affecting their own API.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 29, 2014

Member

I'll want to think more about this, but having everything return error sounds like what should happen, this is Go after all.

I think either way it is okay. If there is an error setting the window title -- there isn't anything you can do about it. Its like if fmt.Println returns an error -- what will you do?

I'd say it's more like Write() or Close() returning errors, which they do.

Member

dmitshur commented Aug 29, 2014

I'll want to think more about this, but having everything return error sounds like what should happen, this is Go after all.

I think either way it is okay. If there is an error setting the window title -- there isn't anything you can do about it. Its like if fmt.Println returns an error -- what will you do?

I'd say it's more like Write() or Close() returning errors, which they do.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 30, 2014

Member

I'd say it's more like Write() or Close() returning errors, which they do.

Valid point. For the record I do agree with the idea of making everything return an error.

Member

slimsag commented Aug 30, 2014

I'd say it's more like Write() or Close() returning errors, which they do.

Valid point. For the record I do agree with the idea of making everything return an error.

@elmindreda

This comment has been minimized.

Show comment
Hide comment
@elmindreda

elmindreda Sep 1, 2014

This implies that it could be possible to run one dedicated thread per window and call MakeContextCurrent on each window in each thread. In that case we would have to register a custom error callback for each thread. Do you have the same perception or have I misunderstood how glfw operates?

I couldn't see this explicitly addressed above, so just in case.

There is only one error callback. It's not stored per thread. It may be called by any thread, but is always called on the thread that caused the error, i.e. the thread that called GLFW.

elmindreda commented Sep 1, 2014

This implies that it could be possible to run one dedicated thread per window and call MakeContextCurrent on each window in each thread. In that case we would have to register a custom error callback for each thread. Do you have the same perception or have I misunderstood how glfw operates?

I couldn't see this explicitly addressed above, so just in case.

There is only one error callback. It's not stored per thread. It may be called by any thread, but is always called on the thread that caused the error, i.e. the thread that called GLFW.

@tapir tapir referenced this issue Sep 3, 2014

Closed

Returning errors #56

@tapir tapir closed this Jan 26, 2015

tapir added a commit that referenced this issue Feb 22, 2015

slimsag added a commit that referenced this issue Feb 22, 2015

Return an error from all functions. See #8
All GLFW functions except glfwSetErrorCallback, glfwGetVersion, and
glfwGetVersionString can generate errors via the GLFW error callback.

@dmitshur dmitshur referenced this issue Mar 15, 2017

Closed

Error query #970

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment