Skip to content
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

Invalid code generated for funcs with unnamed return parameters that defer blocking code. #603

Closed
nwidger opened this issue Mar 13, 2017 · 8 comments
Assignees
Labels

Comments

@nwidger
Copy link

nwidger commented Mar 13, 2017

https://gopherjs.github.io/playground/#/JcN1xwSTbp

If I run this Playground link in Firefox 52.0, the should get here alert dialog appears. If I run the same link in Chrome 56.0.2924.87, it prints calling alert(err) but not returned from alert(err) and the following stack trace is generated:

Uncaught (in promise) TypeError: Cannot read property 'Error' of undefined
    at Object.main [as $blk] (eval at $b (playground.js:66), <anonymous>:2943:14)
    at Object.$init [as $blk] (eval at $b (playground.js:66), <anonymous>:2963:68)
    at fun (eval at $b (playground.js:66), <anonymous>:1477:37)
    at $goroutine (eval at $b (playground.js:66), <anonymous>:1475:19)
    at $runScheduled (eval at $b (playground.js:66), <anonymous>:1515:7)
    at $schedule (eval at $b (playground.js:66), <anonymous>:1531:5)
    at queueEntry (eval at $b (playground.js:66), <anonymous>:1686:11)
    at $send (eval at $b (playground.js:66), <anonymous>:1556:5)
    at $b (eval at $b (playground.js:66), <anonymous>:2895:70432)
    at v.$externalizeWrapper (eval at $b (playground.js:66), <anonymous>:1871:24)

Removing the defer statement after http.Get resolves the issue in Chrome:

https://gopherjs.github.io/playground/#/DK-J3eMXYr

Is using defer like this incorrect? If it is I would expect to either see a compilation error or see the same issue in both Firefox and Chrome.

@dmitshur dmitshur added the bug label Mar 13, 2017
@dmitshur
Copy link
Member

dmitshur commented Mar 13, 2017

I can reproduce in Chrome 57.0.2987.98.

This is definitely a bug and shouldn't be happening.

I've also updated the playground to latest version of GopherJS just now, and it still continues to occur there. So this is a valid issue in master.

@dmitshur
Copy link
Member

dmitshur commented Mar 13, 2017

It seems the value of err is somehow undefined (aka js.Undefined).

https://gopherjs.github.io/playground/#/Q5xYbr2_yf

That prints undefined in browser console in Chrome, which is very unexpected.

image

@nwidger
Copy link
Author

nwidger commented Mar 14, 2017

It definitely seems to be related to io.Copy being called in a defer statement. This does not exhibit the same issue:

https://gopherjs.github.io/playground/#/d-r8POieip

@dmitshur
Copy link
Member

dmitshur commented Mar 14, 2017

I suspect this may be related to the fix in 5010146, and #381, #426, #493 issues it resolved. /cc @neelance

@dmitshur
Copy link
Member

dmitshur commented Mar 14, 2017

Here's a simplified reproduce case:

package main

import "time"

func f() error {
	defer func() {
		time.Sleep(time.Second)
	}()
	return nil
}

func main() {
	err := f()
	print(err) // prints "undefined"
}

@dmitshur
Copy link
Member

Interestingly, it works fine if the return parameter has a name, even if that name is blank identifier.

Both these work ok:

package main

import "fmt"
import "time"

func f() (err error) {
	defer func() {
		time.Sleep(time.Second)
	}()
	return nil
}

func main() {
	err := f()
	fmt.Println(err) // prints "<nil>" which is correct
}
package main

import "fmt"
import "time"

func f() (_ error) {
	defer func() {
		time.Sleep(time.Second)
	}()
	return nil
}

func main() {
	err := f()
	fmt.Println(err) // prints "<nil>" which is correct
}

So, this bug should be easy to fix, it's just an unhandled edge case.

@dmitshur dmitshur changed the title Cannot read property 'Error' of undefined in Chrome, works in Firefox Invalid code generated for funcs with unnamed return parameters that defer blocking code. Mar 14, 2017
dmitshur added a commit to shurcooL/play that referenced this issue Apr 12, 2017
@nevkontakte nevkontakte self-assigned this Jun 13, 2021
@nevkontakte
Copy link
Member

I ran into this bug too and I think I know what's the issue here. Consider the following program:

func TestReturnWithBlockingDefer(t *testing.T) {
	f := func() int {
		defer time.Sleep(0)
		return 42
	}
	got := f()
	if got != 42 {
		t.Errorf("Unexpected return value %v. Want: 42.", got)
	}
}

Here's the code f compiles into:

f = (function $b() {
	var $s, $deferred, $r;
	/* */ $s = 0; var $f, $c = false; if (this !== undefined && this.$blk !== undefined) { $f = this; $c = true; $s = $f.$s; $deferred = $f.$deferred; $r = $f.$r; } var $err = null; try { s: while (true) { switch ($s) { case 0: $deferred = []; $deferred.index = $curGoroutine.deferStack.length; $curGoroutine.deferStack.push($deferred);
	$deferred.push([time.Sleep, [new time.Duration(0, 0)]]);
	$s = -1; return 42;
	/* */ } return; } } catch(err) { $err = err; $s = -1; return 0; } finally { $callDeferred($deferred, $err); if($curGoroutine.asleep) { if ($f === undefined) { $f = { $blk: $b }; } $f.$s = $s; $f.$deferred = $deferred; $f.$r = $r; return $f; } }
});

When the function executes the first time, it successfully passes through the $s = -1; return 42; statement and would've returned 42 as expected if the deferred function didn't get blocked. But the deferred function does get blocked, and when function f is called the second time it's executed with $s == -1 and completely skips the return 42 statement. Instead it passes through the /* */ } return; statement and returns undefined as a result.

@nevkontakte
Copy link
Member

Thinking a bit more about this, the right fix is to treat the return statement as blocking in case any of the deferred functions are blocking. And for a blocking return statement we need to compute the return value, store it in a temporary variable, generate a new resumption case and then returned the stored value.

nevkontakte added a commit to nevkontakte/gopherjs that referenced this issue Jun 13, 2021
nevkontakte added a commit to nevkontakte/gopherjs that referenced this issue Jun 13, 2021
nevkontakte added a commit to nevkontakte/gopherjs that referenced this issue Jun 15, 2021
This doesn't make any changes to the existing logic, but makes the code
a bit easier to read (or so I hope) and explains some of the logic I had
to reverse-engineer in the comments.

Overall this should make fixing
gopherjs#603 a bit easier.
nevkontakte added a commit to nevkontakte/gopherjs that referenced this issue Jun 19, 2021
This doesn't make any changes to the existing logic, but makes the code
a bit easier to read (or so I hope) and explains some of the logic I had
to reverse-engineer in the comments.

Overall this should make fixing
gopherjs#603 a bit easier.
nevkontakte added a commit to nevkontakte/gopherjs that referenced this issue Jun 19, 2021
nevkontakte added a commit to nevkontakte/gopherjs that referenced this issue Jun 19, 2021
Fixes gopherjs#603.

The fix consists of two parts:

 1. If a function has a deferred call, we assume that call might be
    blocking and therefore return statements in the function may behave
    like a blocking call even though the returned expression may not be
    blocking itself.

 2. When generating code for return statements that were marked as
    blocking we add a resumption point to make sure that before
    resuming the deferred function we re-return the correct value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants